From eab380b76b2670dd15bc1ddf92d8052b91a3b066 Mon Sep 17 00:00:00 2001 From: Michael Freno Date: Wed, 29 Apr 2026 00:28:01 -0400 Subject: [PATCH] Fix FRE-622 security findings: IDOR, auth, markdown injection, email validation H-1: Add createdBy to alertRules, IDOR check on update/delete H-2: Add createdBy to scheduledReports, IDOR check on update H-3: Add createdBy to cohorts, IDOR check on addCohortMember M-1: Change submitNPSResponse to protectedProcedure M-2: Escape Slack Markdown special chars in alert rule names M-3: Change getAllLatestKPIs, getAlertRules, getAlerts, getNPSResponses to protectedProcedure L-2: Add email regex validation to recipients field Co-Authored-By: Paperclip --- server/trpc/analytics-router.ts | 65 +++++++++++++++++++++++++--- src/db/schema/alert_rules.ts | 2 + src/db/schema/cohorts.ts | 1 + src/db/schema/scheduled_reports.ts | 2 + src/lib/analytics/cohort-analysis.ts | 2 + src/lib/analytics/slack-alerts.ts | 16 ++++++- 6 files changed, 80 insertions(+), 8 deletions(-) diff --git a/server/trpc/analytics-router.ts b/server/trpc/analytics-router.ts index 982f0c0ee..cdf15b02e 100644 --- a/server/trpc/analytics-router.ts +++ b/server/trpc/analytics-router.ts @@ -88,7 +88,7 @@ export const analyticsRouter = { return await getLatestKPI(ctx.db!, input.kpiKey as KPIKey); }), - getAllLatestKPIs: publicProcedure.query(async ({ ctx }) => { + getAllLatestKPIs: protectedProcedure.query(async ({ ctx }) => { return await getAllLatestKPIs(ctx.db!); }), @@ -147,7 +147,7 @@ export const analyticsRouter = { // --- Alert Endpoints --- - getAlertRules: publicProcedure.query(async ({ ctx }) => { + getAlertRules: protectedProcedure.query(async ({ ctx }) => { return await ctx.db!.select().from(alertRules).orderBy(desc(alertRules.createdAt)); }), @@ -171,6 +171,7 @@ export const analyticsRouter = { channelId: input.channelId ?? null, isActive: true, cooldownMinutes: input.cooldownMinutes, + createdBy: ctx.userId, }).returning(); return result[0]; }), @@ -188,6 +189,18 @@ export const analyticsRouter = { })) .mutation(async ({ input, ctx }) => { const { id, ...updates } = input; + const existing = await ctx.db! + .select({ id: alertRules.id, createdBy: alertRules.createdBy }) + .from(alertRules) + .where(eq(alertRules.id, id)) + .limit(1); + const rule = existing[0]; + if (!rule) { + throw new (await import('./router')).TRPCError({ code: 'NOT_FOUND', message: 'Alert rule not found' }); + } + if (rule.createdBy !== ctx.userId) { + throw new (await import('./router')).TRPCError({ code: 'FORBIDDEN', message: 'Not the rule owner' }); + } const result = await ctx.db! .update(alertRules) .set({ ...updates, updatedAt: new Date() }) @@ -199,11 +212,23 @@ export const analyticsRouter = { deleteAlertRule: protectedProcedure .input(z.object({ id: z.number().int().positive() })) .mutation(async ({ input, ctx }) => { + const existing = await ctx.db! + .select({ id: alertRules.id, createdBy: alertRules.createdBy }) + .from(alertRules) + .where(eq(alertRules.id, input.id)) + .limit(1); + const rule = existing[0]; + if (!rule) { + throw new (await import('./router')).TRPCError({ code: 'NOT_FOUND', message: 'Alert rule not found' }); + } + if (rule.createdBy !== ctx.userId) { + throw new (await import('./router')).TRPCError({ code: 'FORBIDDEN', message: 'Not the rule owner' }); + } await ctx.db!.delete(alertRules).where(eq(alertRules.id, input.id)); return { success: true }; }), - getAlerts: publicProcedure + getAlerts: protectedProcedure .input(z.object({ severity: AlertSeveritySchema.optional(), limit: z.number().int().min(1).max(200).default(50), @@ -274,7 +299,7 @@ export const analyticsRouter = { name: z.string().min(1).max(200), reportType: ReportTypeSchema, schedule: ScheduleSchema, - recipients: z.string(), + recipients: z.string().regex(/^[^\s@]+@[^\s@]+\.[^\s@]+(,[^\s@]+@[^\s@]+\.[^\s@]+)*$/, 'Each recipient must be a valid email'), format: ReportFormatSchema, metadata: z.record(z.string(), z.unknown()).optional(), })) @@ -287,6 +312,7 @@ export const analyticsRouter = { format: input.format, isActive: true, metadata: input.metadata ? JSON.stringify(input.metadata) : null, + createdBy: ctx.userId, }); return report; }), @@ -297,12 +323,24 @@ export const analyticsRouter = { name: z.string().min(1).max(200).optional(), reportType: ReportTypeSchema.optional(), schedule: ScheduleSchema.optional(), - recipients: z.string().optional(), + recipients: z.string().regex(/^[^\s@]+@[^\s@]+\.[^\s@]+(,[^\s@]+@[^\s@]+\.[^\s@]+)*$/, 'Each recipient must be a valid email').optional(), format: ReportFormatSchema.optional(), isActive: z.boolean().optional(), })) .mutation(async ({ input, ctx }) => { const { id, ...updates } = input; + const existing = await ctx.db! + .select({ id: scheduledReports.id, createdBy: scheduledReports.createdBy }) + .from(scheduledReports) + .where(eq(scheduledReports.id, id)) + .limit(1); + const report = existing[0]; + if (!report) { + throw new (await import('./router')).TRPCError({ code: 'NOT_FOUND', message: 'Scheduled report not found' }); + } + if (report.createdBy !== ctx.userId) { + throw new (await import('./router')).TRPCError({ code: 'FORBIDDEN', message: 'Not the report owner' }); + } const result = await ctx.db! .update(scheduledReports) .set({ ...updates, updatedAt: new Date() }) @@ -341,6 +379,7 @@ export const analyticsRouter = { periodStart: new Date(input.periodStart), periodEnd: input.periodEnd ? new Date(input.periodEnd) : undefined, filterCriteria: input.filterCriteria, + createdBy: ctx.userId, }); return cohort; }), @@ -351,6 +390,18 @@ export const analyticsRouter = { userId: z.number().int().positive(), })) .mutation(async ({ input, ctx }) => { + const existing = await ctx.db! + .select({ id: cohorts.id, createdBy: cohorts.createdBy }) + .from(cohorts) + .where(eq(cohorts.id, input.cohortId)) + .limit(1); + const cohort = existing[0]; + if (!cohort) { + throw new (await import('./router')).TRPCError({ code: 'NOT_FOUND', message: 'Cohort not found' }); + } + if (cohort.createdBy !== ctx.userId) { + throw new (await import('./router')).TRPCError({ code: 'FORBIDDEN', message: 'Not the cohort owner' }); + } await addCohortMember(ctx.db!, input.cohortId, input.userId); const size = await getCohortSize(ctx.db!, input.cohortId); return { success: true, cohortSize: size }; @@ -382,7 +433,7 @@ export const analyticsRouter = { // --- NPS Endpoints --- - submitNPSResponse: publicProcedure + submitNPSResponse: protectedProcedure .input(z.object({ score: z.number().int().min(0).max(10), userId: z.number().int().positive().optional(), @@ -414,7 +465,7 @@ export const analyticsRouter = { ); }), - getNPSResponses: publicProcedure + getNPSResponses: protectedProcedure .input(z.object({ category: z.enum(['detractor', 'passive', 'promoter']).optional(), periodStart: z.string().datetime().optional(), diff --git a/src/db/schema/alert_rules.ts b/src/db/schema/alert_rules.ts index 8a50d42f0..75dcd1f71 100644 --- a/src/db/schema/alert_rules.ts +++ b/src/db/schema/alert_rules.ts @@ -1,4 +1,5 @@ import { sqliteTable, text, integer, real } from "drizzle-orm/sqlite-core"; +import { users } from "./users"; export const alertRules = sqliteTable("alert_rules", { id: integer("id").primaryKey({ autoIncrement: true }), @@ -10,6 +11,7 @@ export const alertRules = sqliteTable("alert_rules", { channelId: text("channel_id"), isActive: integer("is_active", { mode: "boolean" }).notNull().default(true), cooldownMinutes: integer("cooldown_minutes").notNull().default(60), + createdBy: integer("created_by").references(() => users.id), createdAt: integer("created_at", { mode: "timestamp" }).notNull().default(new Date()), updatedAt: integer("updated_at", { mode: "timestamp" }).notNull().default(new Date()), }); diff --git a/src/db/schema/cohorts.ts b/src/db/schema/cohorts.ts index 7ac8ecec9..8093d65bf 100644 --- a/src/db/schema/cohorts.ts +++ b/src/db/schema/cohorts.ts @@ -10,6 +10,7 @@ export const cohorts = sqliteTable("cohorts", { size: integer("size").notNull().default(0), retentionData: text("retention_data"), metadata: text("metadata"), + createdBy: integer("created_by").references(() => users.id), createdAt: integer("created_at", { mode: "timestamp" }).notNull().default(new Date()), updatedAt: integer("updated_at", { mode: "timestamp" }).notNull().default(new Date()), }); diff --git a/src/db/schema/scheduled_reports.ts b/src/db/schema/scheduled_reports.ts index 04bfd57b6..544ee1da8 100644 --- a/src/db/schema/scheduled_reports.ts +++ b/src/db/schema/scheduled_reports.ts @@ -1,4 +1,5 @@ import { sqliteTable, text, integer } from "drizzle-orm/sqlite-core"; +import { users } from "./users"; export const scheduledReports = sqliteTable("scheduled_reports", { id: integer("id").primaryKey({ autoIncrement: true }), @@ -11,6 +12,7 @@ export const scheduledReports = sqliteTable("scheduled_reports", { lastRunAt: integer("last_run_at", { mode: "timestamp" }), nextRunAt: integer("next_run_at", { mode: "timestamp" }), metadata: text("metadata"), + createdBy: integer("created_by").references(() => users.id), createdAt: integer("created_at", { mode: "timestamp" }).notNull().default(new Date()), updatedAt: integer("updated_at", { mode: "timestamp" }).notNull().default(new Date()), }); diff --git a/src/lib/analytics/cohort-analysis.ts b/src/lib/analytics/cohort-analysis.ts index 5162a284c..0eb24f31b 100644 --- a/src/lib/analytics/cohort-analysis.ts +++ b/src/lib/analytics/cohort-analysis.ts @@ -9,6 +9,7 @@ export interface CohortDefinition { periodStart: Date; periodEnd?: Date; filterCriteria: Record; + createdBy?: number; } export interface CohortAnalysisResult { @@ -37,6 +38,7 @@ export async function createCohort( size: 0, retentionData: null, metadata: definition.description ? JSON.stringify({ description: definition.description }) : null, + createdBy: definition.createdBy ?? null, }; const result = await db.insert(cohorts).values(cohort).returning(); diff --git a/src/lib/analytics/slack-alerts.ts b/src/lib/analytics/slack-alerts.ts index 78fe6f234..402c246c3 100644 --- a/src/lib/analytics/slack-alerts.ts +++ b/src/lib/analytics/slack-alerts.ts @@ -108,6 +108,18 @@ function mapSeverity( return rule.severity; } +function escapeSlackMarkdown(text: string): string { + return text + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/\*/g, '\\*') + .replace(/_/g, '\\_') + .replace(/~/g, '\\~') + .replace(/`/g, '\\`') + .replace(/\(/g, '\\('); +} + export function formatAlertMessage( rule: typeof alertRules.$inferSelect, value: number, @@ -118,7 +130,9 @@ export function formatAlertMessage( ? rule.threshold.toString() : rule.threshold.toFixed(2); - return `${rule.name}: ${kpiKey} is ${formattedValue} (${rule.condition} ${formattedThreshold})`; + const safeName = escapeSlackMarkdown(rule.name); + + return `${safeName}: ${kpiKey} is ${formattedValue} (${rule.condition} ${formattedThreshold})`; } export async function sendSlackAlert(