diff --git a/src/app/api/ai-insights/route.ts b/src/app/api/ai-insights/route.ts index fca82c172..8c2be6e86 100644 --- a/src/app/api/ai-insights/route.ts +++ b/src/app/api/ai-insights/route.ts @@ -2,18 +2,29 @@ import { NextResponse } from "next/server"; import { getServerSession } from "next-auth"; import { authOptions } from "@/lib/auth"; import { supabaseAdmin } from "@/lib/supabase"; +import { resolveAppUser } from "@/lib/resolve-user"; import { weeklyProductivityPrompt } from "@/lib/ai-prompts"; import { analyzePatterns, computeTrends, DeveloperMetrics, } from "@/lib/ai-mentor"; + const aiInsightsRateLimit = new Map< string, { count: number; resetTime: number } >(); + export const dynamic = "force-dynamic"; +const VALID_INSIGHT_TYPES = new Set([ + "weekly_summary", + "pattern", + "recommendation", +] as const); + +type InsightType = "weekly_summary" | "pattern" | "recommendation"; + interface ContributionsApiResponse { data?: Record; total?: number; @@ -48,7 +59,18 @@ export async function GET(request: Request) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); } - const userId = session.githubId; + // Resolve the application user so that ai_insights rows are keyed on + // users.id (a stable UUID with a foreign-key constraint) rather than on + // session.githubId (a mutable external identifier with no FK). Using + // users.id ensures rows are removed via ON DELETE CASCADE when the account + // is deleted and prevents the orphaned-record scenario described in #1750. + const user = await resolveAppUser(session.githubId, session.githubLogin); + if (!user) { + return NextResponse.json({ error: "User not found" }, { status: 404 }); + } + + const userId = user.id; + const currentTime = Date.now(); const WINDOW_MS = 60 * 60 * 1000; const MAX_REQUESTS = 5; @@ -73,8 +95,19 @@ export async function GET(request: Request) { ); } existing.count += 1; + const { searchParams } = new URL(request.url); - const type = searchParams.get("type") ?? "weekly_summary"; + const rawType = searchParams.get("type") ?? "weekly_summary"; + + // Validate against the DB CHECK constraint allowlist so that an unrecognised + // type fails with a clear 400 instead of a Supabase constraint-violation 500. + if (!VALID_INSIGHT_TYPES.has(rawType as InsightType)) { + return NextResponse.json( + { error: `Invalid insight type. Must be one of: ${[...VALID_INSIGHT_TYPES].join(", ")}` }, + { status: 400 } + ); + } + const type = rawType as InsightType; const { data: cached } = await supabaseAdmin .from("ai_insights") diff --git a/src/app/api/user/data-export/route.ts b/src/app/api/user/data-export/route.ts index 918f3eed2..045ec05c2 100644 --- a/src/app/api/user/data-export/route.ts +++ b/src/app/api/user/data-export/route.ts @@ -319,6 +319,10 @@ export async function DELETE(req: NextRequest) { // Tables with a direct user_id foreign key, ordered to respect any // potential FK constraints (children before parents). + // ai_insights is included explicitly here even though ON DELETE CASCADE on + // the foreign key would remove those rows when the users row is deleted. + // The explicit delete is a defense-in-depth measure that works regardless + // of whether the FK migration has been applied to a given environment. const tablesToDelete = [ "notifications", "ai_insights", diff --git a/supabase/migrations/20260531000000_fix_ai_insights_user_id.sql b/supabase/migrations/20260531000000_fix_ai_insights_user_id.sql new file mode 100644 index 000000000..0ed9f0089 --- /dev/null +++ b/supabase/migrations/20260531000000_fix_ai_insights_user_id.sql @@ -0,0 +1,52 @@ +-- Migration: fix ai_insights ownership and referential integrity +-- +-- Problem +-- ------- +-- ai_insights.user_id was populated with session.githubId (a numeric GitHub +-- account ID stored as text, e.g. "12345678") instead of users.id (the +-- application UUID, e.g. "550e8400-..."). As a result: +-- +-- 1. No foreign-key constraint existed, so rows could reference +-- non-existent "users". +-- 2. ON DELETE CASCADE could not fire because there was no FK relationship, +-- leaving ai_insights rows permanently orphaned after account deletion. +-- 3. The unique index on (user_id, insight_type) operated on GitHub IDs +-- rather than application user IDs, making the constraint meaningless +-- for ownership isolation. +-- +-- This migration: +-- 1. Remaps existing user_id values from github_id → users.id using the +-- users.github_id column that links the two identity spaces. +-- 2. Deletes any rows that cannot be mapped (accounts already deleted). +-- 3. Adds a proper FOREIGN KEY referencing users(id) ON DELETE CASCADE. +-- +-- The unique index idx_ai_insights_user_type already covers (user_id, +-- insight_type) and remains valid after the remap because every user has +-- at most one insight row per type. + +BEGIN; + +-- Step 1: Remap github_id values to application UUIDs. +-- ai_insights.user_id currently holds values equal to users.github_id. +-- After this UPDATE each row will hold the corresponding users.id UUID. +UPDATE ai_insights AS ai +SET user_id = u.id +FROM users AS u +WHERE u.github_id = ai.user_id; + +-- Step 2: Remove rows that could not be matched (the originating user account +-- no longer exists in the database). These are already orphaned records; +-- deleting them here is correct and expected. +DELETE FROM ai_insights +WHERE user_id NOT IN (SELECT id FROM users); + +-- Step 3: Enforce referential integrity going forward. +-- ON DELETE CASCADE ensures rows are removed automatically when their +-- owning user account is deleted, preventing future orphans. +ALTER TABLE ai_insights + ADD CONSTRAINT ai_insights_user_id_fkey + FOREIGN KEY (user_id) + REFERENCES users(id) + ON DELETE CASCADE; + +COMMIT; diff --git a/test/ai-insights-ownership.test.ts b/test/ai-insights-ownership.test.ts new file mode 100644 index 000000000..8726458c6 --- /dev/null +++ b/test/ai-insights-ownership.test.ts @@ -0,0 +1,247 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// ─── hoisted mocks ────────────────────────────────────────────────────────── + +const mocks = vi.hoisted(() => ({ + getServerSession: vi.fn(), + resolveAppUser: vi.fn(), + supabaseFrom: vi.fn(), + fetch: vi.fn(), +})); + +vi.mock("next-auth", () => ({ getServerSession: mocks.getServerSession })); +vi.mock("@/lib/auth", () => ({ authOptions: {} })); +vi.mock("@/lib/resolve-user", () => ({ resolveAppUser: mocks.resolveAppUser })); +vi.mock("@/lib/supabase", () => ({ + supabaseAdmin: { from: mocks.supabaseFrom }, +})); +vi.mock("@/lib/ai-prompts", () => ({ + weeklyProductivityPrompt: vi.fn().mockReturnValue("prompt"), +})); +vi.mock("@/lib/ai-mentor", () => ({ + analyzePatterns: vi.fn().mockReturnValue([]), + computeTrends: vi.fn().mockReturnValue({ direction: "up", percentage: 5 }), +})); + +vi.stubGlobal("fetch", mocks.fetch); + +// ─── helpers ──────────────────────────────────────────────────────────────── + +const GITHUB_ID = "12345678"; + +// Each test that exercises the request path uses a unique UUID so it gets its +// own rate-limit bucket inside the module-level Map. This prevents the 5-req +// per-hour cap from spilling across tests. +let testCounter = 0; +function freshUUID(): string { + testCounter += 1; + return `550e8400-e29b-41d4-a716-${String(testCounter).padStart(12, "0")}`; +} + +function makeRequest(type = "weekly_summary"): Request { + return new Request(`http://localhost/api/ai-insights?type=${type}`, { + headers: { cookie: "next-auth.session-token=tok" }, + }); +} + +function setupSupabaseCacheMiss() { + const upsertChain = vi.fn().mockResolvedValue({ error: null }); + + const maybeSingle = vi.fn().mockResolvedValue({ data: null, error: null }); + const limit = vi.fn().mockReturnValue({ maybeSingle }); + const order = vi.fn().mockReturnValue({ limit }); + const gte = vi.fn().mockReturnValue({ order }); + const eqType = vi.fn().mockReturnValue({ gte }); + const eqUserId = vi.fn().mockReturnValue({ eq: eqType }); + const selectChain = vi.fn().mockReturnValue({ eq: eqUserId }); + + mocks.supabaseFrom.mockReturnValue({ + select: selectChain, + upsert: upsertChain, + }); + + return { upsertChain, selectChain, eqUserId }; +} + +function setupSupabaseCacheHit(content: object) { + const maybeSingle = vi.fn().mockResolvedValue({ data: { content }, error: null }); + const limit = vi.fn().mockReturnValue({ maybeSingle }); + const order = vi.fn().mockReturnValue({ limit }); + const gte = vi.fn().mockReturnValue({ order }); + const eqType = vi.fn().mockReturnValue({ gte }); + const eqUserId = vi.fn().mockReturnValue({ eq: eqType }); + const selectChain = vi.fn().mockReturnValue({ eq: eqUserId }); + + mocks.supabaseFrom.mockReturnValue({ + select: selectChain, + upsert: vi.fn(), + }); +} + +function stubMetricsFetches() { + mocks.fetch.mockResolvedValue({ + ok: true, + json: async () => ({}), + text: async () => "{}", + status: 200, + }); +} + +// ─── tests ─────────────────────────────────────────────────────────────────── + +describe("GET /api/ai-insights — ownership model", () => { + beforeEach(() => { + vi.clearAllMocks(); + stubMetricsFetches(); + + mocks.getServerSession.mockResolvedValue({ + githubId: GITHUB_ID, + githubLogin: "alice", + }); + // Default: resolved to a fresh UUID per test + mocks.resolveAppUser.mockResolvedValue({ id: freshUUID() }); + }); + + // ── authentication ──────────────────────────────────────────────────────── + + it("returns 401 when there is no session", async () => { + mocks.getServerSession.mockResolvedValue(null); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest()); + expect(res.status).toBe(401); + }); + + it("returns 404 when resolveAppUser returns null", async () => { + mocks.resolveAppUser.mockResolvedValue(null); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest()); + expect(res.status).toBe(404); + }); + + // ── ownership: user.id not githubId ────────────────────────────────────── + + it("calls resolveAppUser with the session githubId and githubLogin", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + await GET(makeRequest()); + expect(mocks.resolveAppUser).toHaveBeenCalledWith(GITHUB_ID, "alice"); + }); + + it("stores insights with users.id (UUID), not session.githubId (numeric string)", async () => { + const uuid = freshUUID(); + mocks.resolveAppUser.mockResolvedValue({ id: uuid }); + const { upsertChain } = setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + await GET(makeRequest()); + + expect(upsertChain).toHaveBeenCalledWith( + expect.objectContaining({ user_id: uuid }), + expect.any(Object) + ); + // Must NOT use the GitHub numeric ID as the user_id + expect(upsertChain).not.toHaveBeenCalledWith( + expect.objectContaining({ user_id: GITHUB_ID }), + expect.any(Object) + ); + }); + + it("queries the cache with users.id, not githubId", async () => { + const uuid = freshUUID(); + mocks.resolveAppUser.mockResolvedValue({ id: uuid }); + const { eqUserId } = setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + await GET(makeRequest()); + + // The first .eq() in the select chain filters by user_id + expect(eqUserId).toHaveBeenCalledWith("user_id", uuid); + }); + + // ── regression: orphaned-record scenario (#1750) ────────────────────────── + + it("does not use githubId as the user_id in any DB operation", async () => { + const uuid = freshUUID(); + mocks.resolveAppUser.mockResolvedValue({ id: uuid }); + const { upsertChain } = setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + await GET(makeRequest()); + + const upsertArgs = upsertChain.mock.calls.flatMap((call) => call); + const upsertedIds = upsertArgs + .filter((a) => a && typeof a === "object" && "user_id" in a) + .map((a: any) => a.user_id); + + expect(upsertedIds).not.toContain(GITHUB_ID); + upsertedIds.forEach((id: string) => expect(id).toBe(uuid)); + }); + + // ── cache-hit path ──────────────────────────────────────────────────────── + + it("returns cached insight when a fresh record exists", async () => { + const cachedContent = { insights: [], trend: {}, aiSummary: null }; + setupSupabaseCacheHit(cachedContent); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest()); + const body = await res.json(); + + expect(res.status).toBe(200); + expect(body.cached).toBe(true); + expect(body.data).toEqual(cachedContent); + }); + + // ── insight_type validation ─────────────────────────────────────────────── + + it("returns 400 for an unrecognised insight type", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest("totally_wrong")); + expect(res.status).toBe(400); + const body = await res.json(); + expect(body.error).toMatch(/invalid insight type/i); + }); + + it("accepts weekly_summary as a valid type", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest("weekly_summary")); + expect(res.status).toBe(200); + }); + + it("accepts pattern as a valid type", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest("pattern")); + expect(res.status).toBe(200); + }); + + it("accepts recommendation as a valid type", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest("recommendation")); + expect(res.status).toBe(200); + }); + + // ── upsert conflict key ─────────────────────────────────────────────────── + + it("upserts with user_id,insight_type conflict target", async () => { + const { upsertChain } = setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + await GET(makeRequest()); + + expect(upsertChain).toHaveBeenCalledWith( + expect.any(Object), + { onConflict: "user_id,insight_type" } + ); + }); + + // ── full cache-miss response shape ──────────────────────────────────────── + + it("returns { data, cached: false } on a cache miss", async () => { + setupSupabaseCacheMiss(); + const { GET } = await import("@/app/api/ai-insights/route"); + const res = await GET(makeRequest()); + const body = await res.json(); + expect(res.status).toBe(200); + expect(body.cached).toBe(false); + expect(body.data).toBeDefined(); + }); +});