diff --git a/supabase/migrations/20260716192819_enable_rls.sql b/supabase/migrations/20260716192819_enable_rls.sql new file mode 100644 index 00000000..04b34cc6 --- /dev/null +++ b/supabase/migrations/20260716192819_enable_rls.sql @@ -0,0 +1,211 @@ +-- Enable Row Level Security on every public application table and define +-- owner-scoped access policies. +-- +-- WHY THIS EXISTS +-- Supabase's PostgREST auto-exposes every table in the `public` schema over +-- HTTP, authorized by the *public* anon key (NEXT_PUBLIC_SUPABASE_ANON_KEY, +-- shipped to every browser). With RLS disabled, anyone holding that key can +-- read and modify all rows in these tables (workspaces, prompts, documents, +-- prolific study IDs, ...). Enabling RLS closes that hole. +-- +-- WHY IT DOES NOT BREAK THE APP +-- The workbench never reads/writes these tables through PostgREST. All table +-- access goes through Drizzle over DATABASE_URL as the `postgres` role, which +-- has BYPASSRLS; supabase-js is used only for `.auth` and `.storage`. The +-- service_role key (used by the test suite) also bypasses RLS. We deliberately +-- do NOT use FORCE ROW LEVEL SECURITY, so the table-owning `postgres` role and +-- service_role continue to bypass — exactly the roles the app and tests use. +-- +-- OWNERSHIP GRAPH +-- `workspaces.user_id` = `auth.uid()::text` is the root of ownership. Every +-- other table inherits ownership by walking back to its workspace (directly via +-- workspace_id, or via chart_id -> charts -> workspace). Policies are scoped +-- `TO authenticated`; `anon` matches no policy and is therefore denied on all +-- tables. auth.uid() is wrapped in a scalar sub-select so Postgres caches it as +-- an initplan (Supabase's recommended RLS performance pattern). +-- +-- NOT INCLUDED (by design) +-- No anon-readable policy for `workspaces.public = true`. That sharing path is +-- not served over PostgREST today, and a blanket table policy would expose +-- user_id / prolific columns. Public sharing over the API, if ever needed, +-- should be a column-limited view, not a table policy. +-- +-- ORPHAN TABLES +-- Some tables exist in the live DB but not in the Drizzle schema (e.g. +-- `generations`, left over from the removed generation panel). PostgREST +-- exposes those too. So rather than enable RLS on a hand-listed set, we enable +-- it on EVERY base table in `public` — this self-heals against current and +-- future orphans, locking them to default-deny (bypass roles only) until +-- someone gives them an explicit policy. The owner-scoped policies below then +-- layer onto the known application tables. +-- +-- Idempotent: safe to re-run (enable-rls is a no-op if already on; policies are +-- dropped-if-exists before creation). + +begin; + +-- ── Enable RLS on every public base table (covers known + orphan tables) ───── +do $$ +declare + t text; +begin + for t in + select tablename from pg_tables where schemaname = 'public' + loop + execute format('alter table public.%I enable row level security', t); + end loop; +end $$; + +-- ── workspaces : the ownership root ────────────────────────────────────────── +drop policy if exists workspaces_owner_all on public.workspaces; +create policy workspaces_owner_all on public.workspaces + for all + to authenticated + using (user_id = (select auth.uid())::text) + with check (user_id = (select auth.uid())::text); + +-- ── charts : owned via workspace_id ────────────────────────────────────────── +drop policy if exists charts_owner_all on public.charts; +create policy charts_owner_all on public.charts + for all + to authenticated + using ( + exists ( + select 1 from public.workspaces w + where w.id = charts.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.workspaces w + where w.id = charts.workspace_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── configs : owned via workspace_id ───────────────────────────────────────── +drop policy if exists configs_owner_all on public.configs; +create policy configs_owner_all on public.configs + for all + to authenticated + using ( + exists ( + select 1 from public.workspaces w + where w.id = configs.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.workspaces w + where w.id = configs.workspace_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── documents : owned via workspace_id ─────────────────────────────────────── +drop policy if exists documents_owner_all on public.documents; +create policy documents_owner_all on public.documents + for all + to authenticated + using ( + exists ( + select 1 from public.workspaces w + where w.id = documents.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.workspaces w + where w.id = documents.workspace_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── lens_runs : owned via workspace_id, and chart_id must agree ────────────── +-- Validating workspace_id alone would let an owner insert a run under their +-- workspace while pointing chart_id at another user's chart; join charts and +-- require both columns resolve to the same owned workspace. +drop policy if exists lens_runs_owner_all on public.lens_runs; +create policy lens_runs_owner_all on public.lens_runs + for all + to authenticated + using ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + where c.id = lens_runs.chart_id + and c.workspace_id = lens_runs.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + where c.id = lens_runs.chart_id + and c.workspace_id = lens_runs.workspace_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── views : owned via chart_id -> charts -> workspace ──────────────────────── +drop policy if exists views_owner_all on public.views; +create policy views_owner_all on public.views + for all + to authenticated + using ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + where c.id = views.chart_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + where c.id = views.chart_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── chart_config_links : chart AND config must share one owned workspace ───── +-- Validating chart_id alone would let an owner link their chart to another +-- user's config (cross-tenant disclosure via copyChart); require the config to +-- live in the same owned workspace as the chart. +drop policy if exists chart_config_links_owner_all on public.chart_config_links; +create policy chart_config_links_owner_all on public.chart_config_links + for all + to authenticated + using ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + join public.configs cfg on cfg.id = chart_config_links.config_id + where c.id = chart_config_links.chart_id + and cfg.workspace_id = c.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.charts c + join public.workspaces w on w.id = c.workspace_id + join public.configs cfg on cfg.id = chart_config_links.config_id + where c.id = chart_config_links.chart_id + and cfg.workspace_id = c.workspace_id + and w.user_id = (select auth.uid())::text + ) + ); + +-- ── workshops : admin-managed metadata, no client access ───────────────────── +-- Created and read only through server actions (Drizzle `postgres` role) and +-- the /w/{slug} join flow (also server-side). RLS is already enabled by the +-- do-loop above; with NO policy defined, anon and authenticated are both fully +-- denied over PostgREST — only bypass roles reach it. No ALTER needed here. + +commit; diff --git a/workbench/_web/src/actions/workshop.ts b/workbench/_web/src/actions/workshop.ts index 66262dda..906a606d 100644 --- a/workbench/_web/src/actions/workshop.ts +++ b/workbench/_web/src/actions/workshop.ts @@ -102,7 +102,7 @@ export async function joinWorkshopAction( // insert conflicts and it reuses the winner's workspace. let workspace; try { - workspace = await createWorkspace(user.id, workshop.name, workshop.id, prolific); + workspace = await createWorkspace(workshop.name, workshop.id, prolific); } catch (err) { if (!isUniqueViolation(err)) throw err; const winner = await getWorkshopWorkspaceForUser(user.id, workshop.id); diff --git a/workbench/_web/src/app/workbench/components/AutoWorkspaceCreator.tsx b/workbench/_web/src/app/workbench/components/AutoWorkspaceCreator.tsx index 3e3bddc0..f4c00f47 100644 --- a/workbench/_web/src/app/workbench/components/AutoWorkspaceCreator.tsx +++ b/workbench/_web/src/app/workbench/components/AutoWorkspaceCreator.tsx @@ -16,7 +16,6 @@ import type { Lens2ConfigData } from "@/types/lens2"; import type { ActivationPatchingConfigData, SourcePosition } from "@/types/activationPatching"; interface AutoWorkspaceCreatorProps { - userId: string; initialPrompt?: string; initialModel?: string; seedWithExamples?: boolean; // New prop to control seeding @@ -34,7 +33,6 @@ interface AutoWorkspaceCreatorProps { } export function AutoWorkspaceCreator({ - userId, initialPrompt, initialModel, seedWithExamples = true, // Default to true for new users @@ -68,13 +66,8 @@ export function AutoWorkspaceCreator({ console.log("Using existing workspace:", existingWorkspaceId); targetWorkspaceId = existingWorkspaceId; } else { - console.log( - "Creating workspace for user:", - userId, - "with name:", - workspaceName, - ); - const newWorkspace = await createWorkspace(userId, workspaceName); + console.log("Creating workspace with name:", workspaceName); + const newWorkspace = await createWorkspace(workspaceName); console.log("Created workspace:", newWorkspace); targetWorkspaceId = newWorkspace.id; @@ -201,7 +194,6 @@ export function AutoWorkspaceCreator({ createAndRedirect(); }, [ - userId, router, initialPrompt, initialModel, diff --git a/workbench/_web/src/app/workbench/components/WorkspaceList.tsx b/workbench/_web/src/app/workbench/components/WorkspaceList.tsx index d9dc45ef..dd22a010 100644 --- a/workbench/_web/src/app/workbench/components/WorkspaceList.tsx +++ b/workbench/_web/src/app/workbench/components/WorkspaceList.tsx @@ -10,14 +10,11 @@ import { Trash2, BarChart3, FileText, ChevronLeft, ChevronRight } from "lucide-r import { useEffect, useMemo, useState } from "react"; import { useIsDark } from "@/hooks/useIsDark"; import { useModelsSection } from "@/stores/useModelsSection"; +import { queryKeys } from "@/lib/queryKeys"; const PAGE_SIZE_EXPANDED = 8; // 2 rows × 4 cols at lg+, 4 rows × 2 cols at sm const PAGE_SIZE_COLLAPSED = 16; // 4 rows × 4 cols at lg+ — uses the freed vertical space -interface WorkspaceListProps { - userId: string; -} - interface Workspace { id: string; name: string; @@ -123,12 +120,12 @@ function WorkspaceCard({ ); } -export function WorkspaceList({ userId }: WorkspaceListProps) { +export function WorkspaceList() { const deleteWorkspaceMutation = useDeleteWorkspace(); const { data: workspaces, isLoading } = useQuery({ - queryKey: ["workspaces"], - queryFn: () => getWorkspaces(userId), + queryKey: queryKeys.workspaces.all, + queryFn: () => getWorkspaces(), staleTime: 0, }); @@ -155,7 +152,7 @@ export function WorkspaceList({ userId }: WorkspaceListProps) { e.preventDefault(); e.stopPropagation(); if (confirm("Are you sure you want to delete this workspace?")) { - deleteWorkspaceMutation.mutate({ userId, workspaceId }); + deleteWorkspaceMutation.mutate({ workspaceId }); } }; @@ -171,7 +168,7 @@ export function WorkspaceList({ userId }: WorkspaceListProps) { <>

Workspaces

- +
{!workspaces || workspaces.length === 0 ? ( diff --git a/workbench/_web/src/app/workbench/page.tsx b/workbench/_web/src/app/workbench/page.tsx index 413ad762..021b5415 100644 --- a/workbench/_web/src/app/workbench/page.tsx +++ b/workbench/_web/src/app/workbench/page.tsx @@ -3,7 +3,7 @@ import type { User } from "@supabase/supabase-js"; import { ModelsSection } from "@/components/models/ModelsSection"; import { ModelsSectionStateController } from "@/app/workbench/components/ModelsSectionStateController"; import { WorkspaceList } from "@/app/workbench/components/WorkspaceList"; -import { getWorkspaces, createWorkspace } from "@/lib/queries/workspaceQueries"; +import { getWorkspaces } from "@/lib/queries/workspaceQueries"; import { AutoWorkspaceCreator } from "@/app/workbench/components/AutoWorkspaceCreator"; import { PendingRequestHandler } from "@/app/workbench/components/PendingRequestHandler"; import Link from "next/link"; @@ -49,7 +49,7 @@ export default async function WorkbenchPage({ hasWorkshopClaim(user); // Check if user has any workspaces - const workspaces = await getWorkspaces(user.id); + const workspaces = await getWorkspaces(); // Get the prompt and model from search params const params = await searchParams; @@ -141,7 +141,6 @@ export default async function WorkbenchPage({ {useExistingWorkspace ? ( ) : shouldCreateWorkspace ? ( ) : ( - + )} diff --git a/workbench/_web/src/components/CreateWorkspaceDialog.tsx b/workbench/_web/src/components/CreateWorkspaceDialog.tsx index 7511dbc1..67d32fe0 100644 --- a/workbench/_web/src/components/CreateWorkspaceDialog.tsx +++ b/workbench/_web/src/components/CreateWorkspaceDialog.tsx @@ -17,11 +17,7 @@ import { Label } from "@/components/ui/label"; import { useCreateWorkspace } from "@/lib/api/workspaceApi"; import { useRouter } from "next/navigation"; -interface CreateWorkspaceDialogProps { - userId: string; -} - -export function CreateWorkspaceDialog({ userId }: CreateWorkspaceDialogProps) { +export function CreateWorkspaceDialog() { const [open, setOpen] = useState(false); const [name, setName] = useState(""); const router = useRouter(); @@ -33,7 +29,6 @@ export function CreateWorkspaceDialog({ userId }: CreateWorkspaceDialogProps) { try { const newWorkspace = await createWorkspaceMutation.mutateAsync({ - userId, name: name.trim(), }); diff --git a/workbench/_web/src/components/LandingPage.tsx b/workbench/_web/src/components/LandingPage.tsx index 1e85f0d5..03ba00af 100644 --- a/workbench/_web/src/components/LandingPage.tsx +++ b/workbench/_web/src/components/LandingPage.tsx @@ -240,7 +240,7 @@ export function LandingPage({ loggedIn }: { loggedIn: boolean }) { const { data: workspacesList } = useQuery({ queryKey: ["workspaces", currentUser?.id], - queryFn: () => getWorkspaces(currentUser!.id), + queryFn: () => getWorkspaces(), enabled: !!isSignedInUser, }); diff --git a/workbench/_web/src/components/WorkspaceNameEditor.tsx b/workbench/_web/src/components/WorkspaceNameEditor.tsx index 2ff7ac6d..0dc8436b 100644 --- a/workbench/_web/src/components/WorkspaceNameEditor.tsx +++ b/workbench/_web/src/components/WorkspaceNameEditor.tsx @@ -6,29 +6,15 @@ import { useQuery } from "@tanstack/react-query"; import { getWorkspaceById } from "@/lib/queries/workspaceQueries"; import { useUpdateWorkspaceName } from "@/lib/api/workspaceApi"; import { queryKeys } from "@/lib/queryKeys"; -import { createClient } from "@/lib/supabase/client"; import { Loader2 } from "lucide-react"; export function WorkspaceNameEditor() { const { workspaceId } = useParams<{ workspaceId: string }>(); const [localName, setLocalName] = useState(null); const [isEditing, setIsEditing] = useState(false); - const [userId, setUserId] = useState(null); const inputRef = useRef(null); const saveTimeoutRef = useRef(null); - // Get user ID - useEffect(() => { - const getUser = async () => { - const supabase = createClient(); - const { - data: { user }, - } = await supabase.auth.getUser(); - setUserId(user?.id ?? null); - }; - getUser(); - }, []); - // Fetch workspace data const { data: workspace, isLoading } = useQuery({ queryKey: queryKeys.workspaces.workspace(workspaceId), @@ -59,7 +45,7 @@ export function WorkspaceNameEditor() { // Save name (debounced) const saveName = useCallback( (newName: string) => { - if (!workspaceId || !userId) return; + if (!workspaceId) return; if (saveTimeoutRef.current) { clearTimeout(saveTimeoutRef.current); @@ -67,11 +53,11 @@ export function WorkspaceNameEditor() { saveTimeoutRef.current = setTimeout(() => { if (newName.trim()) { - updateName({ workspaceId, name: newName.trim(), userId }); + updateName({ workspaceId, name: newName.trim() }); } }, 500); }, - [workspaceId, userId, updateName], + [workspaceId, updateName], ); // Handle name change diff --git a/workbench/_web/src/components/models/ModelLaunchDialog.tsx b/workbench/_web/src/components/models/ModelLaunchDialog.tsx index 55bca5a1..c51b7b72 100644 --- a/workbench/_web/src/components/models/ModelLaunchDialog.tsx +++ b/workbench/_web/src/components/models/ModelLaunchDialog.tsx @@ -78,7 +78,7 @@ export function ModelLaunchDialog({ model, mode, onOpenChange }: ModelLaunchDial const { data: workspaces } = useQuery({ queryKey: ["workspaces", user?.id], - queryFn: () => getWorkspaces(user!.id), + queryFn: () => getWorkspaces(), enabled: open && isSignedIn && !!user?.id, }); diff --git a/workbench/_web/src/db/__tests__/lensRuns.test.ts b/workbench/_web/src/db/__tests__/lensRuns.test.ts index fce96dcb..984c507f 100644 --- a/workbench/_web/src/db/__tests__/lensRuns.test.ts +++ b/workbench/_web/src/db/__tests__/lensRuns.test.ts @@ -18,12 +18,29 @@ import { deleteLensRun, updateLensRunIntervention, } from "@/lib/queries/lensRunQueries"; +import { createWorkspace } from "@/lib/queries/workspaceQueries"; +import { createLensChartPair } from "@/lib/queries/chartQueries"; +import { setDevUserId } from "@/lib/auth/devUser"; +import { Metrics } from "@/types/lens"; +import type { LensConfigData } from "@/types/lens"; import type { LensRunSummary, LensRunHeatmaps, LensRunPromptSummary } from "@/types/lensRun"; import type { LogitLensIntroData } from "@/types/logitLensIntro"; -const WS = "ws-test-1"; -const CHART_A = "chart-aaaa"; -const CHART_B = "chart-bbbb"; +const USER = "lens-run-user"; + +// lens_runs are owner-scoped via their workspace, and createLensRun verifies the +// caller owns the parent chart — so these tests build a real workspace + two +// charts and use their ids rather than synthetic strings. +let WS: string; +let CHART_A: string; +let CHART_B: string; + +const lensConfig = (prompt: string): LensConfigData => ({ + prompt, + model: "gpt2", + statisticType: Metrics.PROBABILITY, + token: { idx: 0, id: 0, text: "", targetIds: [] }, +}); // A minimal but well-formed full lens payload (1 layer, 1 token). const fakeLens = (finalToken: string): LogitLensIntroData => @@ -64,11 +81,19 @@ const heatmaps = (srcTok = " Paris", tgtTok?: string): LensRunHeatmaps => ({ describe("lens_runs (F1 prompt history)", () => { beforeEach(async () => { await clearDatabase(); + setDevUserId(USER); + const ws = await createWorkspace("Lens Run Workspace"); + WS = ws.id; + const [{ chart: a }, { chart: b }] = [ + await createLensChartPair(WS, lensConfig("chart a")), + await createLensChartPair(WS, lensConfig("chart b")), + ]; + CHART_A = a.id; + CHART_B = b.id; }); it("creates a run and reads back the compact summary payload intact", async () => { const created = await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "meta-llama/Llama-3.1-8B", summary: summary(" Paris", " Rome"), @@ -86,7 +111,6 @@ describe("lens_runs (F1 prompt history)", () => { it("never returns the heavy `data` heatmaps from list queries", async () => { await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" Paris"), @@ -98,7 +122,6 @@ describe("lens_runs (F1 prompt history)", () => { it("fetches full heatmaps on demand by id", async () => { const created = await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), @@ -122,7 +145,6 @@ describe("lens_runs (F1 prompt history)", () => { it("attaches a patch to an existing run via updateLensRunIntervention", async () => { const created = await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), @@ -150,7 +172,6 @@ describe("lens_runs (F1 prompt history)", () => { it("returns runs oldest → newest for a chart (createdAt asc, id asc)", async () => { for (const tok of [" A", " B", " C"]) { await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(tok), @@ -164,21 +185,18 @@ describe("lens_runs (F1 prompt history)", () => { it("scopes history by chart and (optionally) model", async () => { await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m2", summary: summary(" y"), heatmaps: heatmaps(" y"), }); await createLensRun({ - workspaceId: WS, chartId: CHART_B, model: "m1", summary: summary(" z"), @@ -194,14 +212,12 @@ describe("lens_runs (F1 prompt history)", () => { it("clears a chart's history without touching another chart", async () => { await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); await createLensRun({ - workspaceId: WS, chartId: CHART_B, model: "m1", summary: summary(" y"), @@ -215,7 +231,6 @@ describe("lens_runs (F1 prompt history)", () => { it("deletes a single run by id", async () => { const a = await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" x"), @@ -223,7 +238,6 @@ describe("lens_runs (F1 prompt history)", () => { }); await new Promise((r) => setTimeout(r, 5)); await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(" y"), @@ -241,7 +255,6 @@ describe("lens_runs (F1 prompt history)", () => { const total = RETENTION_CAP + 3; for (let i = 0; i < total; i++) { await createLensRun({ - workspaceId: WS, chartId: CHART_A, model: "m1", summary: summary(` t${i}`), @@ -261,4 +274,63 @@ describe("lens_runs (F1 prompt history)", () => { expect(tokens).not.toContain(" t2"); expect(tokens).toContain(` t${total - 1}`); }); + + // The outer beforeEach owns WS/CHART_A as USER; these switch to an attacker + // to prove the chart-derived authorization contract holds across every op. + describe("ownership (cross-user isolation)", () => { + const ATTACKER = "lens-run-attacker"; + + // Seed one run in the victim's chart, then return its id. + const seedVictimRun = async (): Promise => { + const run = await createLensRun({ + chartId: CHART_A, + model: "m1", + summary: summary(" Paris"), + heatmaps: heatmaps(), + }); + return run.id; + }; + + it("rejects createLensRun against another user's chart", async () => { + setDevUserId(ATTACKER); + await expect( + createLensRun({ + chartId: CHART_A, + model: "m1", + summary: summary(" Paris"), + heatmaps: heatmaps(), + }), + ).rejects.toThrow(/not found or access denied/i); + }); + + it("hides another user's runs from list and heatmap fetches", async () => { + const runId = await seedVictimRun(); + + setDevUserId(ATTACKER); + expect(await getLensRunsByChart(WS, CHART_A)).toHaveLength(0); + expect(await getLensRunHeatmaps(runId)).toBeNull(); + expect(await getLensRunHeatmapsByIds([runId])).toHaveLength(0); + }); + + it("does not let another user patch, clear, or delete the victim's runs", async () => { + const runId = await seedVictimRun(); + + setDevUserId(ATTACKER); + await updateLensRunIntervention( + runId, + { kind: "noop" } as never, + promptSummary("x", " y"), + fakeLens(" y"), + ); + await clearLensRunsForChart(WS, CHART_A); + await deleteLensRun(WS, runId); + + // The owner still sees an untouched run. + setDevUserId(USER); + const rows = await getLensRunsByChart(WS, CHART_A); + expect(rows).toHaveLength(1); + expect(rows[0].summary.source.finalToken).toBe(" Paris"); + expect(rows[0].summary.intervention).toBeUndefined(); + }); + }); }); diff --git a/workbench/_web/src/db/__tests__/local-db.test.ts b/workbench/_web/src/db/__tests__/local-db.test.ts index f49a5e97..cce37b9d 100644 --- a/workbench/_web/src/db/__tests__/local-db.test.ts +++ b/workbench/_web/src/db/__tests__/local-db.test.ts @@ -11,6 +11,7 @@ import { describe, it, expect, beforeEach } from "bun:test"; import { db, clearDatabase } from "../client"; +import { setDevUserId } from "@/lib/auth/devUser"; // Import actual query functions import { @@ -31,6 +32,7 @@ import { getConfigForChart, getMostRecentChartForWorkspace, getChartsMetadata, + getAllChartsByType, copyChart, } from "@/lib/queries/chartQueries"; @@ -66,6 +68,10 @@ const createTestLensConfig = (prompt: string = "test"): LensConfigData => ({ beforeEach(async () => { // Clear all tables before each test await clearDatabase(); + // The scoped query actions derive the caller via requireUserId(), which under + // DISABLE_AUTH returns the dev identity. Point it at the id these tests own + // their rows under so the folded ownership predicates match. + setDevUserId(TEST_USER_ID); }); describe("Database Client", () => { @@ -77,7 +83,7 @@ describe("Database Client", () => { describe("Workspace Queries", () => { it("should create a workspace with auto-generated UUID", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "Test Workspace"); + const workspace = await createWorkspace("Test Workspace"); expect(workspace).toBeDefined(); expect(workspace.id).toMatch( @@ -88,7 +94,7 @@ describe("Workspace Queries", () => { }); it("should get workspace by ID", async () => { - const created = await createWorkspace(TEST_USER_ID, "Find Me"); + const created = await createWorkspace("Find Me"); const found = await getWorkspaceById(created.id); expect(found).not.toBeNull(); @@ -102,8 +108,8 @@ describe("Workspace Queries", () => { it("should get all workspaces for a user with chart and document counts", async () => { // Create workspaces - const ws1 = await createWorkspace(TEST_USER_ID, "Workspace 1"); - const ws2 = await createWorkspace(TEST_USER_ID, "Workspace 2"); + const ws1 = await createWorkspace("Workspace 1"); + const ws2 = await createWorkspace("Workspace 2"); // Add charts to workspace 1 await createLensChartPair(ws1.id, createTestLensConfig()); @@ -112,7 +118,7 @@ describe("Workspace Queries", () => { // Add document to workspace 2 await createDocument(ws2.id); - const workspaces = await getWorkspaces(TEST_USER_ID); + const workspaces = await getWorkspaces(); expect(workspaces).toHaveLength(2); @@ -126,37 +132,44 @@ describe("Workspace Queries", () => { }); it("should update a workspace", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "Original Name"); + const workspace = await createWorkspace("Original Name"); - const updated = await updateWorkspace( - workspace.id, - { name: "Updated Name", public: true }, - TEST_USER_ID, - ); + const updated = await updateWorkspace(workspace.id, { + name: "Updated Name", + public: true, + }); expect(updated.name).toBe("Updated Name"); expect(updated.public).toBe(true); }); it("should not update workspace for wrong user", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "My Workspace"); + const workspace = await createWorkspace("My Workspace"); - await expect( - updateWorkspace(workspace.id, { name: "Hacked" }, "wrong-user"), - ).rejects.toThrow("Workspace not found or access denied"); + // A different caller can't satisfy the folded owner predicate: zero rows + // match and the update throws. + setDevUserId("wrong-user"); + await expect(updateWorkspace(workspace.id, { name: "Hacked" })).rejects.toThrow( + "Workspace not found or access denied", + ); + setDevUserId(TEST_USER_ID); }); it("should delete a workspace", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "To Delete"); - await deleteWorkspace(TEST_USER_ID, workspace.id); + const workspace = await createWorkspace("To Delete"); + await deleteWorkspace(workspace.id); const found = await getWorkspaceById(workspace.id); expect(found).toBeNull(); }); it("should not delete workspace for wrong user", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "Protected"); - await deleteWorkspace("wrong-user", workspace.id); + const workspace = await createWorkspace("Protected"); + + // A different caller's delete matches zero rows — the workspace survives. + setDevUserId("wrong-user"); + await deleteWorkspace(workspace.id); + setDevUserId(TEST_USER_ID); const found = await getWorkspaceById(workspace.id); expect(found).not.toBeNull(); @@ -168,7 +181,7 @@ describe("Chart Queries", () => { beforeEach(async () => { await clearDatabase(); - const workspace = await createWorkspace(TEST_USER_ID, "Charts Test Workspace"); + const workspace = await createWorkspace("Charts Test Workspace"); workspaceId = workspace.id; }); @@ -308,7 +321,7 @@ describe("Config Queries", () => { beforeEach(async () => { await clearDatabase(); - const workspace = await createWorkspace(TEST_USER_ID, "Config Test Workspace"); + const workspace = await createWorkspace("Config Test Workspace"); workspaceId = workspace.id; const { chart } = await createLensChartPair(workspaceId, createTestLensConfig()); chartId = chart.id; @@ -359,7 +372,7 @@ describe("View Queries", () => { beforeEach(async () => { await clearDatabase(); - const workspace = await createWorkspace(TEST_USER_ID, "View Test Workspace"); + const workspace = await createWorkspace("View Test Workspace"); workspaceId = workspace.id; const { chart } = await createLensChartPair(workspaceId, createTestLensConfig()); chartId = chart.id; @@ -396,7 +409,7 @@ describe("View Queries", () => { const updated = await updateView(view.id, { updated: true, zoom: 2 } as any); - expect(updated.data).toEqual({ updated: true, zoom: 2 }); + expect(updated!.data).toEqual({ updated: true, zoom: 2 }); }); it("should delete a view", async () => { @@ -413,7 +426,7 @@ describe("Document Queries", () => { beforeEach(async () => { await clearDatabase(); - const workspace = await createWorkspace(TEST_USER_ID, "Document Test Workspace"); + const workspace = await createWorkspace("Document Test Workspace"); workspaceId = workspace.id; }); @@ -488,7 +501,7 @@ describe("JSON Storage in SQLite", () => { beforeEach(async () => { await clearDatabase(); - const workspace = await createWorkspace(TEST_USER_ID, "JSON Test Workspace"); + const workspace = await createWorkspace("JSON Test Workspace"); workspaceId = workspace.id; }); @@ -549,7 +562,7 @@ describe("JSON Storage in SQLite", () => { describe("Cross-Table Relationships", () => { it("should maintain workspace -> charts -> configs relationship", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "Relationship Test"); + const workspace = await createWorkspace("Relationship Test"); // Create multiple charts with configs const { chart: chart1 } = await createLensChartPair( @@ -571,12 +584,12 @@ describe("Cross-Table Relationships", () => { expect(config2!.type).toBe("patch"); // Verify workspace count includes charts - const workspaces = await getWorkspaces(TEST_USER_ID); + const workspaces = await getWorkspaces(); expect(workspaces[0].chartCount).toBe(2); }); it("should handle multiple documents per workspace", async () => { - const workspace = await createWorkspace(TEST_USER_ID, "Multi-Doc Test"); + const workspace = await createWorkspace("Multi-Doc Test"); await createDocument(workspace.id); await createDocument(workspace.id); @@ -585,7 +598,86 @@ describe("Cross-Table Relationships", () => { const docs = await getDocumentsForWorkspace(workspace.id); expect(docs).toHaveLength(3); - const workspaces = await getWorkspaces(TEST_USER_ID); + const workspaces = await getWorkspaces(); expect(workspaces[0].documentCount).toBe(3); }); }); + +describe("Ownership enforcement (scoped queries reject other users)", () => { + const OTHER_USER_ID = "attacker-999"; + let workspaceId: string; + let chartId: string; + + beforeEach(async () => { + // The module-level beforeEach already clears the DB and sets the dev user + // to TEST_USER_ID; this hook only adds the victim fixtures. + const workspace = await createWorkspace("Victim Workspace"); + workspaceId = workspace.id; + const { chart } = await createLensChartPair(workspaceId, createTestLensConfig()); + chartId = chart.id; + }); + + it("hides another user's workspace from getWorkspaceById", async () => { + setDevUserId(OTHER_USER_ID); + expect(await getWorkspaceById(workspaceId)).toBeNull(); + }); + + it("hides another user's chart from getChartById", async () => { + setDevUserId(OTHER_USER_ID); + expect(await getChartById(chartId)).toBeNull(); + }); + + it("does not delete another user's chart", async () => { + setDevUserId(OTHER_USER_ID); + await deleteChart(chartId); // no-op: predicate matches nothing + + // The owner can still see it. + setDevUserId(TEST_USER_ID); + expect(await getChartById(chartId)).not.toBeNull(); + }); + + it("does not update another user's chart name", async () => { + setDevUserId(OTHER_USER_ID); + await updateChartName(chartId, "hacked"); + + setDevUserId(TEST_USER_ID); + const chart = await getChartById(chartId); + expect(chart!.name).not.toBe("hacked"); + }); + + it("refuses to create a chart in another user's workspace", async () => { + setDevUserId(OTHER_USER_ID); + await expect(createLensChartPair(workspaceId, createTestLensConfig())).rejects.toThrow( + /not found or access denied/i, + ); + }); + + it("refuses to create a document in another user's workspace", async () => { + setDevUserId(OTHER_USER_ID); + await expect(createDocument(workspaceId)).rejects.toThrow(/not found or access denied/i); + }); + + it("keeps another user's charts out of getChartsMetadata / getAllChartsByType", async () => { + setDevUserId(OTHER_USER_ID); + expect(await getChartsMetadata(workspaceId)).toHaveLength(0); + // Un-scoped (no workspaceId) must still only return the caller's charts. + const byType = await getAllChartsByType(); + expect(Object.values(byType).flat()).toHaveLength(0); + }); + + it("keeps another user's workspaces out of getWorkspaces", async () => { + setDevUserId(OTHER_USER_ID); + // The attacker sees only their own list — never the victim's workspace. + expect(await getWorkspaces()).toHaveLength(0); + }); + + it("stamps the calling user as owner (createWorkspace ignores any client identity)", async () => { + setDevUserId(OTHER_USER_ID); + const mine = await createWorkspace("Attacker's own"); + // It lands under the caller's identity, and the victim never sees it. + expect(await getWorkspaceById(mine.id)).not.toBeNull(); + + setDevUserId(TEST_USER_ID); + expect(await getWorkspaceById(mine.id)).toBeNull(); + }); +}); diff --git a/workbench/_web/src/db/__tests__/prolific.test.ts b/workbench/_web/src/db/__tests__/prolific.test.ts index 40e4040a..fda4b919 100644 --- a/workbench/_web/src/db/__tests__/prolific.test.ts +++ b/workbench/_web/src/db/__tests__/prolific.test.ts @@ -14,6 +14,7 @@ import { getWorkspaceById, setWorkspaceProlificIfEmpty, } from "@/lib/queries/workspaceQueries"; +import { setDevUserId } from "@/lib/auth/devUser"; const USER = "anon-user-1"; @@ -52,24 +53,26 @@ describe("parseProlificParams", () => { describe("workspace Prolific persistence", () => { beforeEach(async () => { await clearDatabase(); + // getWorkspaceById is owner-scoped; act as the user these rows belong to. + setDevUserId(USER); }); it("stores Prolific params on the workspace at creation", async () => { const params = { prolificPid: "pid-1", studyId: "study-1", sessionId: "sess-1" }; - const ws = await createWorkspace(USER, "Faculty Pilot", undefined, params); + const ws = await createWorkspace("Faculty Pilot", undefined, params); const fetched = await getWorkspaceById(ws.id); expect(fetched!.prolific).toEqual(params); }); it("defaults prolific to null when none are captured", async () => { - const ws = await createWorkspace(USER, "Personal"); + const ws = await createWorkspace("Personal"); const fetched = await getWorkspaceById(ws.id); expect(fetched!.prolific).toBeNull(); }); it("backfills params onto a workspace that has none", async () => { - const ws = await createWorkspace(USER, "Joined without params"); + const ws = await createWorkspace("Joined without params"); expect((await getWorkspaceById(ws.id))!.prolific).toBeNull(); const params = { prolificPid: "late-pid" }; @@ -79,9 +82,23 @@ describe("workspace Prolific persistence", () => { it("first-touch wins: backfill does not clobber existing params", async () => { const original = { prolificPid: "first", studyId: "study-1" }; - const ws = await createWorkspace(USER, "Faculty Pilot", undefined, original); + const ws = await createWorkspace("Faculty Pilot", undefined, original); await setWorkspaceProlificIfEmpty(ws.id, { prolificPid: "second" }); expect((await getWorkspaceById(ws.id))!.prolific).toEqual(original); }); + + it("rejects a non-owner backfilling Prolific params onto someone else's workspace", async () => { + const ws = await createWorkspace("Victim"); + + // A different caller must not be able to stamp attribution onto a + // workspace they don't own — the ownership guard rejects it. + setDevUserId("attacker-999"); + await expect( + setWorkspaceProlificIfEmpty(ws.id, { prolificPid: "planted" }), + ).rejects.toThrow("Workspace not found or access denied"); + + setDevUserId(USER); + expect((await getWorkspaceById(ws.id))!.prolific).toBeNull(); + }); }); diff --git a/workbench/_web/src/db/__tests__/workshops.test.ts b/workbench/_web/src/db/__tests__/workshops.test.ts index aecfe1b4..5ab4bee4 100644 --- a/workbench/_web/src/db/__tests__/workshops.test.ts +++ b/workbench/_web/src/db/__tests__/workshops.test.ts @@ -21,6 +21,7 @@ import { deleteWorkshop, } from "@/lib/queries/workshopDb"; import { createWorkspace, getWorkspaceById } from "@/lib/queries/workspaceQueries"; +import { setDevUserId } from "@/lib/auth/devUser"; import type { WorkshopTool } from "@/db/schema"; const USER = "anon-user-1"; @@ -39,6 +40,8 @@ const input = (overrides: Partial[0]> = {}) => describe("workshops", () => { beforeEach(async () => { await clearDatabase(); + // getWorkspaceById is owner-scoped; act as the participant user. + setDevUserId(USER); }); it("creates a workshop and reads it back by slug", async () => { @@ -83,8 +86,8 @@ describe("workshops", () => { it("resolves a workspace's workshop through workshopId, null otherwise", async () => { const workshop = await createWorkshop(input()); - const stamped = await createWorkspace(USER, "Faculty Pilot", workshop.id); - const plain = await createWorkspace(USER, "Personal"); + const stamped = await createWorkspace("Faculty Pilot", workshop.id); + const plain = await createWorkspace("Personal"); const found = await getWorkshopForWorkspace(stamped.id); expect(found).not.toBeNull(); @@ -99,9 +102,13 @@ describe("workshops", () => { expect(await getWorkshopWorkspaceForUser(USER, workshop.id)).toBeNull(); - const ws = await createWorkspace(USER, "Faculty Pilot", workshop.id); - await createWorkspace(USER, "Other Session", other.id); - await createWorkspace("someone-else", "Their Session", workshop.id); + const ws = await createWorkspace("Faculty Pilot", workshop.id); + await createWorkspace("Other Session", other.id); + // A different participant's workspace in the same workshop — created under + // their own identity so it belongs to them, not USER. + setDevUserId("someone-else"); + await createWorkspace("Their Session", workshop.id); + setDevUserId(USER); const found = await getWorkshopWorkspaceForUser(USER, workshop.id); expect(found).not.toBeNull(); @@ -110,22 +117,26 @@ describe("workshops", () => { it("enforces one workspace per (user, workshop); NULL workshopId is unconstrained", async () => { const workshop = await createWorkshop(input()); - await createWorkspace(USER, "First join", workshop.id); + await createWorkspace("First join", workshop.id); // Second insert for the same pair conflicts — this is what makes // concurrent join-link clicks converge in joinWorkshopAction. - await expect(createWorkspace(USER, "Racing join", workshop.id)).rejects.toThrow(/unique/i); + await expect(createWorkspace("Racing join", workshop.id)).rejects.toThrow(/unique/i); // Normal workspaces (NULL workshopId) stay unconstrained. - await createWorkspace(USER, "Personal 1"); - await createWorkspace(USER, "Personal 2"); + await createWorkspace("Personal 1"); + await createWorkspace("Personal 2"); }); it("lists workshops with participant counts", async () => { const a = await createWorkshop(input({ name: "A" })); const b = await createWorkshop(input({ name: "B" })); - await createWorkspace("u1", "A ws", a.id); - await createWorkspace("u2", "A ws", a.id); + // Two distinct participants each join workshop A, under their own identity. + setDevUserId("u1"); + await createWorkspace("A ws", a.id); + setDevUserId("u2"); + await createWorkspace("A ws", a.id); + setDevUserId(USER); const list = await listWorkshops(); expect(list.length).toBe(2); @@ -151,7 +162,7 @@ describe("workshops", () => { it("delete nulls participant workspaces' workshopId (workspace survives)", async () => { const workshop = await createWorkshop(input()); - const ws = await createWorkspace(USER, "Faculty Pilot", workshop.id); + const ws = await createWorkspace("Faculty Pilot", workshop.id); await deleteWorkshop(workshop.id); diff --git a/workbench/_web/src/lib/api/patchLensApi.ts b/workbench/_web/src/lib/api/patchLensApi.ts index d27c6766..b525f82a 100644 --- a/workbench/_web/src/lib/api/patchLensApi.ts +++ b/workbench/_web/src/lib/api/patchLensApi.ts @@ -141,7 +141,6 @@ export const runPatchLensLogitLens = async ( ...(target ? { target } : {}), }; const created = await createLensRun({ - workspaceId: request.workspaceId, chartId: request.chartId, model: request.model, summary, diff --git a/workbench/_web/src/lib/api/workspaceApi.ts b/workbench/_web/src/lib/api/workspaceApi.ts index dec28228..e3cd2c31 100644 --- a/workbench/_web/src/lib/api/workspaceApi.ts +++ b/workbench/_web/src/lib/api/workspaceApi.ts @@ -14,13 +14,13 @@ export const useCreateWorkspace = () => { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async ({ userId, name }: { userId: string; name: string }) => { - // This calls the server action which handles authentication - const newWorkspace = await createWorkspace(userId, name); + mutationFn: async ({ name }: { name: string }) => { + // The server action derives the caller from the session. + const newWorkspace = await createWorkspace(name); return newWorkspace; }, onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["workspaces"] }); + queryClient.invalidateQueries({ queryKey: queryKeys.workspaces.all }); console.log("Successfully created workspace"); }, onError: (error) => { @@ -33,11 +33,11 @@ export const useDeleteWorkspace = () => { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async ({ userId, workspaceId }: { userId: string; workspaceId: string }) => { - await deleteWorkspace(userId, workspaceId); + mutationFn: async ({ workspaceId }: { workspaceId: string }) => { + await deleteWorkspace(workspaceId); }, onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["workspaces"] }); + queryClient.invalidateQueries({ queryKey: queryKeys.workspaces.all }); console.log("Successfully deleted workspace"); }, onError: (error) => { @@ -114,16 +114,8 @@ export const useUpdateWorkspaceName = () => { const queryClient = useQueryClient(); return useMutation({ - mutationFn: async ({ - workspaceId, - name, - userId, - }: { - workspaceId: string; - name: string; - userId: string; - }) => { - const updated = await updateWorkspace(workspaceId, { name }, userId); + mutationFn: async ({ workspaceId, name }: { workspaceId: string; name: string }) => { + const updated = await updateWorkspace(workspaceId, { name }); return updated; }, onSuccess: (data, variables) => { diff --git a/workbench/_web/src/lib/auth/devUser.ts b/workbench/_web/src/lib/auth/devUser.ts new file mode 100644 index 00000000..763244d6 --- /dev/null +++ b/workbench/_web/src/lib/auth/devUser.ts @@ -0,0 +1,20 @@ +/** + * The synthetic user id used by ownership guards when auth is disabled + * (NEXT_PUBLIC_DISABLE_AUTH=true) — local dev and the bun:test DB suite. + * + * Defaults to the same stub id the Supabase mock client returns in + * src/lib/supabase/server.ts, so local dev behaves as a single logged-in user. + * Tests flip it via setDevUserId() to act as different users and exercise the + * ownership guards (there's no real session to switch under DISABLE_AUTH). + * In production DISABLE_AUTH is never set, so this module is inert. + */ +let devUserId = "local-dev-user"; + +export function getDevUserId(): string { + return devUserId; +} + +/** Test/dev only: set the acting user id for the DISABLE_AUTH identity. */ +export function setDevUserId(id: string): void { + devUserId = id; +} diff --git a/workbench/_web/src/lib/auth/ownership.ts b/workbench/_web/src/lib/auth/ownership.ts new file mode 100644 index 00000000..bb9e16fc --- /dev/null +++ b/workbench/_web/src/lib/auth/ownership.ts @@ -0,0 +1,134 @@ +/** + * Authorization for the per-user entity graph. + * + * The Drizzle server actions run as the `postgres` role, which bypasses RLS, so + * the database enforces nothing for the app path — authorization has to happen + * here. Every "use server" export is a publicly callable RPC endpoint (see the + * note in ./admin.ts), so a server action that touches a user-owned row must + * derive the caller from the session (never trust a client-supplied userId) and + * ensure the row belongs to them. + * + * Two enforcement shapes, picked by whether there's a row to filter: + * + * - READS / UPDATES / DELETES fold an ownership predicate straight into the + * statement's WHERE (`ownedByWorkspace` / `ownedByChart`). No pre-flight + * SELECT — the ownership check *is* the query, so an unowned id simply + * matches nothing (read → null/empty, write → 0 rows). One round-trip. + * + * - INSERTS have no row to filter, so they verify the *parent* is owned first + * (`requireWorkspaceOwner` / `requireChartOwner`) — the one place a separate + * SELECT is unavoidable. + * + * Ownership roots at `workspaces.user_id`. Child entities resolve their owner by + * walking back to the workspace: charts/configs/documents/lens_runs via + * `workspace_id`; views via `chart_id -> charts`. + */ +import { db } from "@/db/client"; +import { workspaces, charts } from "@/db/schema"; +import { and, eq, exists, sql, type AnyColumn } from "drizzle-orm"; + +/** Thrown when the caller is unauthenticated or doesn't own the target row. */ +export class ForbiddenError extends Error { + constructor(message = "Forbidden") { + super(message); + this.name = "ForbiddenError"; + } +} + +/** + * The authenticated user's id, or throw. Under NEXT_PUBLIC_DISABLE_AUTH (local + * dev + tests) this is the settable dev identity and no Supabase client is + * constructed — which also keeps next/headers `cookies()` out of the bun:test + * path. In production it reads the SSR session. + */ +export async function requireUserId(): Promise { + if (process.env.NEXT_PUBLIC_DISABLE_AUTH === "true") { + // Fail closed: DISABLE_AUTH hands every public server action the synthetic + // dev identity, so it must never be honored in a production build — a + // misconfigured deploy would otherwise bypass session auth entirely. + if (process.env.NODE_ENV === "production") { + throw new ForbiddenError("Authentication cannot be disabled in production"); + } + // Lazy import so the dev-identity module isn't bundled into prod paths. + const { getDevUserId } = await import("./devUser"); + return getDevUserId(); + } + // Perf note: getUser() is a network round-trip to Supabase Auth, and each + // guarded RPC is a separate request, so opening a chart (getChartById + + // getConfigForChart + getView + getChartsMetadata) spends one round-trip per + // action. Within-request memoization wouldn't help (they're separate + // requests); collapsing this to a single per-navigation validation is an + // auth-architecture change left as a follow-up. + // + // Lazy import so the Next-only supabase/server module (and its next/headers + // dependency) is never pulled into the DISABLE_AUTH / test path. + const { createClient } = await import("@/lib/supabase/server"); + const supabase = await createClient(); + const { + data: { user }, + } = await supabase.auth.getUser(); + if (!user) throw new ForbiddenError("Not authenticated"); + return user.id; +} + +/** + * SQL predicate: the row whose workspace-fk column is `workspaceIdCol` is owned + * by `userId`. A correlated `EXISTS` against `workspaces` — folds into any + * WHERE on a table that carries a `workspace_id`, on both dialects. + */ +export function ownedByWorkspace(workspaceIdCol: AnyColumn, userId: string) { + return exists( + db + .select({ one: sql`1` }) + .from(workspaces) + .where(and(eq(workspaces.id, workspaceIdCol), eq(workspaces.userId, userId))), + ); +} + +/** + * SQL predicate: the row whose chart-fk column is `chartIdCol` is owned by + * `userId`, resolved via `chart -> workspace`. For tables that reference a chart + * (views) rather than a workspace directly. + */ +export function ownedByChart(chartIdCol: AnyColumn, userId: string) { + return exists( + db + .select({ one: sql`1` }) + .from(charts) + .innerJoin(workspaces, eq(charts.workspaceId, workspaces.id)) + .where(and(eq(charts.id, chartIdCol), eq(workspaces.userId, userId))), + ); +} + +/** + * Assert the caller owns `workspaceId`; returns the caller's user id. For + * INSERTs that need to confirm the parent workspace before writing a child. + */ +export async function requireWorkspaceOwner(workspaceId: string): Promise { + const userId = await requireUserId(); + const [row] = await db + .select({ id: workspaces.id }) + .from(workspaces) + .where(and(eq(workspaces.id, workspaceId), eq(workspaces.userId, userId))) + .limit(1); + if (!row) throw new ForbiddenError("Workspace not found or access denied"); + return userId; +} + +/** + * Assert the caller owns the chart's workspace; returns id + workspaceId. For + * INSERTs/copies that hang a new row off an existing chart. + */ +export async function requireChartOwner( + chartId: string, +): Promise<{ userId: string; workspaceId: string }> { + const userId = await requireUserId(); + const [row] = await db + .select({ workspaceId: charts.workspaceId }) + .from(charts) + .innerJoin(workspaces, eq(charts.workspaceId, workspaces.id)) + .where(and(eq(charts.id, chartId), eq(workspaces.userId, userId))) + .limit(1); + if (!row) throw new ForbiddenError("Chart not found or access denied"); + return { userId, workspaceId: row.workspaceId }; +} diff --git a/workbench/_web/src/lib/queries/chartQueries.ts b/workbench/_web/src/lib/queries/chartQueries.ts index 147b5059..f5acf72b 100644 --- a/workbench/_web/src/lib/queries/chartQueries.ts +++ b/workbench/_web/src/lib/queries/chartQueries.ts @@ -2,55 +2,95 @@ import type { ChartData, ChartMetadata, ChartView, ChartType, ToolType } from "@/types/charts"; import { db } from "@/db/client"; -import { charts, configs, chartConfigLinks, Chart, LensConfig, Config } from "@/db/schema"; +import { + charts, + configs, + chartConfigLinks, + workspaces, + Chart, + LensConfig, + Config, +} from "@/db/schema"; import { LensConfigData } from "@/types/lens"; import { Lens2ConfigData } from "@/types/lens2"; import { PatchingConfig } from "@/types/patching"; import { ActivationPatchingConfigData } from "@/types/activationPatching"; import { PatchLensChartData } from "@/types/patchLens"; -import { eq, asc, desc, sql } from "drizzle-orm"; -import { touchWorkspace, getNextWorkspaceItemPosition } from "@/lib/queries/workspaceQueries"; +import { eq, and, asc, desc, sql } from "drizzle-orm"; +import { touchWorkspace, nextWorkspaceItemPositionSql } from "@/lib/queries/internal"; +import { + requireUserId, + requireWorkspaceOwner, + ownedByWorkspace, + ForbiddenError, +} from "@/lib/auth/ownership"; // From workshopDb (not workshopQueries) — workshopQueries imports the chart // pair creators below, so importing it back here would be circular. import { getWorkshopForWorkspace } from "@/lib/queries/workshopDb"; export const setChartData = async (chartId: string, chartData: ChartData, chartType: ChartType) => { + const userId = await requireUserId(); const [chart] = await db .update(charts) .set({ data: chartData, type: chartType }) - .where(eq(charts.id, chartId)) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))) .returning(); if (chart) await touchWorkspace(chart.workspaceId); }; export const updateChartName = async (chartId: string, name: string) => { - await db.update(charts).set({ name }).where(eq(charts.id, chartId)); + const userId = await requireUserId(); + await db + .update(charts) + .set({ name }) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))); }; export const getChartById = async (chartId: string): Promise => { - const [chart] = await db.select().from(charts).where(eq(charts.id, chartId)); + const userId = await requireUserId(); + const [chart] = await db + .select() + .from(charts) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))); return (chart ?? null) as Chart | null; }; export const getChartView = async (chartId: string): Promise => { - const [chart] = await db.select().from(charts).where(eq(charts.id, chartId)); + const userId = await requireUserId(); + const [chart] = await db + .select() + .from(charts) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))); return (chart?.view ?? null) as ChartView | null; }; export const updateChartView = async (chartId: string, view: ChartView) => { - await db.update(charts).set({ view }).where(eq(charts.id, chartId)); + const userId = await requireUserId(); + await db + .update(charts) + .set({ view }) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))); }; export const deleteChart = async (chartId: string): Promise => { - await db.delete(charts).where(eq(charts.id, chartId)); + const userId = await requireUserId(); + await db + .delete(charts) + .where(and(eq(charts.id, chartId), ownedByWorkspace(charts.workspaceId, userId))); }; export const getConfigForChart = async (chartId: string): Promise => { + const userId = await requireUserId(); const rows = await db .select() .from(configs) .innerJoin(chartConfigLinks, eq(configs.id, chartConfigLinks.configId)) - .where(eq(chartConfigLinks.chartId, chartId)) + .where( + and( + eq(chartConfigLinks.chartId, chartId), + ownedByWorkspace(configs.workspaceId, userId), + ), + ) .limit(1); if (rows.length === 0) return null; return rows[0].configs as Config; @@ -73,6 +113,9 @@ const createChartConfigPair = async ( // patch-lens chart flows through here. chartData?: ChartData, ): Promise<{ chart: Chart; config: Config }> => { + // INSERT: no row to filter, so verify the parent workspace is owned before + // writing the chart/config/link into it. + await requireWorkspaceOwner(workspaceId); // Workshop workspaces only allow their configured tools. The sidebar // filters its buttons, but every create wrapper here is a public server // action, so the allowlist is enforced at the single shared entry point. @@ -80,10 +123,13 @@ const createChartConfigPair = async ( if (workshop && !(workshop.allowedTools as string[]).includes(payload.type)) { throw new Error(`This workshop does not allow the "${payload.type}" tool`); } - const position = await getNextWorkspaceItemPosition(workspaceId); const [newChart] = await db .insert(charts) - .values({ workspaceId, position, ...(chartData !== undefined ? { data: chartData } : {}) }) + .values({ + workspaceId, + position: nextWorkspaceItemPositionSql(workspaceId), + ...(chartData !== undefined ? { data: chartData } : {}), + }) .returning(); const [newConfig] = await db .insert(configs) @@ -121,7 +167,10 @@ export const createActivationPatchingChartPair = async ( export const getAllChartsByType = async ( workspaceId?: string, ): Promise> => { - // Join charts with their configs to get the config type + const userId = await requireUserId(); + // Join charts with their configs to get the config type. Always scoped to + // the caller's charts (the workspaceId arg only narrows further) — without + // it, an omitted workspaceId would return every user's charts. const query = db .select({ chart: charts, @@ -131,9 +180,10 @@ export const getAllChartsByType = async ( .leftJoin(chartConfigLinks, eq(charts.id, chartConfigLinks.chartId)) .leftJoin(configs, eq(chartConfigLinks.configId, configs.id)); + const ownership = ownedByWorkspace(charts.workspaceId, userId); const chartsWithConfigs = workspaceId - ? await query.where(eq(charts.workspaceId, workspaceId)) - : await query; + ? await query.where(and(eq(charts.workspaceId, workspaceId), ownership)) + : await query.where(ownership); // Group charts by their config type const chartsByType: Record = {}; @@ -150,6 +200,7 @@ export const getAllChartsByType = async ( }; export const getChartsMetadata = async (workspaceId: string): Promise => { + const userId = await requireUserId(); // Whether the chart has a saved result, derived cheaply from the column // being non-null (a freshly created chart has no `data` until it runs) so // we don't ship the heavy result payload into the lightweight sidebar list. @@ -175,7 +226,9 @@ export const getChartsMetadata = async (workspaceId: string): Promise => { + const userId = await requireUserId(); const [chart] = await db .select() .from(charts) - .where(eq(charts.workspaceId, workspaceId)) + .where( + and(eq(charts.workspaceId, workspaceId), ownedByWorkspace(charts.workspaceId, userId)), + ) .orderBy(desc(charts.updatedAt)) .limit(1); @@ -228,13 +284,25 @@ export const getMostRecentChartForWorkspace = async ( }; export const copyChart = async (chartId: string): Promise => { - // Get the original chart - const [originalChart] = await db.select().from(charts).where(eq(charts.id, chartId)); - if (!originalChart) { - throw new Error("Chart not found"); + // INSERT rooted at an existing chart: the copy lands in the same (owned) + // workspace. One ownership-scoped full-row fetch — the join to workspaces both + // authorizes the caller and returns every column the copy needs. + const userId = await requireUserId(); + const [row] = await db + .select() + .from(charts) + .innerJoin(workspaces, eq(charts.workspaceId, workspaces.id)) + .where(and(eq(charts.id, chartId), eq(workspaces.userId, userId))) + .limit(1); + if (!row) { + throw new ForbiddenError("Chart not found or access denied"); } + const originalChart = row.charts; - // Get the config associated with the original chart + // Get the config associated with the original chart, scoped to the chart's + // own (owned) workspace. A link created by the previously-unguarded + // addChartConfigLink could point at another tenant's config; refusing to copy + // a cross-workspace config keeps the disclosure closed here too. const [originalLink] = await db .select() .from(chartConfigLinks) @@ -243,7 +311,15 @@ export const copyChart = async (chartId: string): Promise => { const [originalConfig] = await db .select() .from(configs) - .where(eq(configs.id, originalLink.configId)); + .where( + and( + eq(configs.id, originalLink.configId), + eq(configs.workspaceId, originalChart.workspaceId), + ), + ); + if (!originalConfig) { + throw new ForbiddenError("Config not found or access denied"); + } // Create the new chart with copied data. For patch-lens, drop the pointer to // the source chart's active lens run so the copy starts without history. @@ -254,7 +330,6 @@ export const copyChart = async (chartId: string): Promise => { copiedData = rest as typeof originalChart.data; } - const position = await getNextWorkspaceItemPosition(originalChart.workspaceId); const [newChart] = await db .insert(charts) .values({ @@ -263,7 +338,7 @@ export const copyChart = async (chartId: string): Promise => { data: copiedData, type: originalChart.type, view: originalChart.view, - position, + position: nextWorkspaceItemPositionSql(originalChart.workspaceId), }) .returning(); diff --git a/workbench/_web/src/lib/queries/configQueries.ts b/workbench/_web/src/lib/queries/configQueries.ts index c85e5187..2e91b81e 100644 --- a/workbench/_web/src/lib/queries/configQueries.ts +++ b/workbench/_web/src/lib/queries/configQueries.ts @@ -1,30 +1,69 @@ "use server"; import { db } from "@/db/client"; -import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; import { configs, NewConfig, Config, chartConfigLinks } from "@/db/schema"; +import { + requireUserId, + requireWorkspaceOwner, + requireChartOwner, + ownedByWorkspace, + ForbiddenError, +} from "@/lib/auth/ownership"; export const setConfig = async (configId: string, config: NewConfig): Promise => { - await db.update(configs).set(config).where(eq(configs.id, configId)); + const userId = await requireUserId(); + // Only the payload/type are mutable. Never spread the whole `config`: that + // would let a client rewrite `workspaceId` (a with-check bypass) and relocate + // an owned config into another workspace, silently detaching it from its chart. + await db + .update(configs) + .set({ data: config.data, type: config.type }) + .where(and(eq(configs.id, configId), ownedByWorkspace(configs.workspaceId, userId))); }; export const addConfig = async (config: NewConfig): Promise => { + // INSERT: confirm the target workspace is owned before writing the config. + await requireWorkspaceOwner(config.workspaceId); await db.insert(configs).values(config); }; export const deleteConfig = async (configId: string): Promise => { - await db.delete(configs).where(eq(configs.id, configId)); + const userId = await requireUserId(); + await db + .delete(configs) + .where(and(eq(configs.id, configId), ownedByWorkspace(configs.workspaceId, userId))); }; export const addChartConfigLink = async (configId: string, chartId: string): Promise => { + // INSERT: linking a config to a chart. The caller must own the chart, AND the + // config must live in that same owned workspace — chart ownership alone + // doesn't establish config ownership, so without this a caller could link + // their chart to another tenant's config and read it back (e.g. via copyChart). + const { workspaceId } = await requireChartOwner(chartId); + const [config] = await db + .select({ id: configs.id }) + .from(configs) + .where(and(eq(configs.id, configId), eq(configs.workspaceId, workspaceId))) + .limit(1); + if (!config) throw new ForbiddenError("Config not found or access denied"); await db.insert(chartConfigLinks).values({ configId, chartId }); }; export const getConfigs = async (chartId: string): Promise => { + const userId = await requireUserId(); const configsData = await db .select() .from(configs) .innerJoin(chartConfigLinks, eq(configs.id, chartConfigLinks.configId)) - .where(eq(chartConfigLinks.chartId, chartId)); - return configsData.map((data) => data.configs); + .where( + and( + eq(chartConfigLinks.chartId, chartId), + ownedByWorkspace(configs.workspaceId, userId), + ), + ); + // Explicit element type: the correlated EXISTS in the WHERE tips drizzle's + // dual-schema inference into widening the joined row to `any` (same quirk as + // getChartsMetadata). The projection is unchanged, so the cast is safe. + return (configsData as { configs: Config }[]).map((data) => data.configs); }; diff --git a/workbench/_web/src/lib/queries/documentQueries.ts b/workbench/_web/src/lib/queries/documentQueries.ts index ad0447a9..340dfdc6 100644 --- a/workbench/_web/src/lib/queries/documentQueries.ts +++ b/workbench/_web/src/lib/queries/documentQueries.ts @@ -2,21 +2,37 @@ import { db } from "@/db/client"; import { documents, Document } from "@/db/schema"; -import { asc, eq } from "drizzle-orm"; +import { and, asc, eq } from "drizzle-orm"; import { SerializedEditorState } from "lexical"; -import { touchWorkspace, getNextWorkspaceItemPosition } from "@/lib/queries/workspaceQueries"; +import { touchWorkspace, nextWorkspaceItemPositionSql } from "@/lib/queries/internal"; +import { + requireUserId, + requireWorkspaceOwner, + ownedByWorkspace, + ForbiddenError, +} from "@/lib/auth/ownership"; export const getDocumentById = async (documentId: string): Promise => { - const [document] = await db.select().from(documents).where(eq(documents.id, documentId)); + const userId = await requireUserId(); + const [document] = await db + .select() + .from(documents) + .where(and(eq(documents.id, documentId), ownedByWorkspace(documents.workspaceId, userId))); return document ?? null; }; export const getDocumentByWorkspaceId = async (workspaceId: string): Promise => { + const userId = await requireUserId(); const [document] = await db .select() .from(documents) - .where(eq(documents.workspaceId, workspaceId)); + .where( + and( + eq(documents.workspaceId, workspaceId), + ownedByWorkspace(documents.workspaceId, userId), + ), + ); return document ?? null; }; @@ -26,11 +42,13 @@ export const updateDocument = async ( documentId: string, content: SerializedEditorState, ): Promise => { + const userId = await requireUserId(); const [updated] = await db .update(documents) .set({ content }) - .where(eq(documents.id, documentId)) + .where(and(eq(documents.id, documentId), ownedByWorkspace(documents.workspaceId, userId))) .returning(); + if (!updated) throw new ForbiddenError("Document not found or access denied"); await touchWorkspace(updated.workspaceId); return updated; }; @@ -100,13 +118,21 @@ export type DocumentListItem = Pick => { + const userId = await requireUserId(); const docs = await db .select() .from(documents) - .where(eq(documents.workspaceId, workspaceId)) + .where( + and( + eq(documents.workspaceId, workspaceId), + ownedByWorkspace(documents.workspaceId, userId), + ), + ) .orderBy(asc(documents.position), asc(documents.createdAt)); - return docs.map((d) => ({ + // Explicit element type: the correlated EXISTS widens drizzle's inferred row + // to `any` (same quirk as getChartsMetadata); the projection is unchanged. + return (docs as Document[]).map((d) => ({ id: d.id, workspaceId: d.workspaceId, position: d.position, @@ -156,15 +182,16 @@ const defaultInitialContent = { } as unknown as SerializedEditorState; export const createDocument = async (workspaceId: string): Promise => { + // INSERT: confirm the parent workspace is owned before adding a document. + await requireWorkspaceOwner(workspaceId); const initialContent = defaultInitialContent; - const position = await getNextWorkspaceItemPosition(workspaceId); const [document] = await db .insert(documents) .values({ workspaceId, content: initialContent, - position, + position: nextWorkspaceItemPositionSql(workspaceId), }) .returning(); @@ -173,5 +200,8 @@ export const createDocument = async (workspaceId: string): Promise => }; export const deleteDocument = async (documentId: string): Promise => { - await db.delete(documents).where(eq(documents.id, documentId)); + const userId = await requireUserId(); + await db + .delete(documents) + .where(and(eq(documents.id, documentId), ownedByWorkspace(documents.workspaceId, userId))); }; diff --git a/workbench/_web/src/lib/queries/internal.ts b/workbench/_web/src/lib/queries/internal.ts new file mode 100644 index 00000000..45b01c05 --- /dev/null +++ b/workbench/_web/src/lib/queries/internal.ts @@ -0,0 +1,45 @@ +/** + * Unguarded query internals — NOT a "use server" module, so these are ordinary + * imports rather than publicly callable RPC endpoints. They're helpers the + * guarded server actions call *after* an ownership check has already passed + * (e.g. a create wrapper that has verified the parent workspace), so they carry + * no guard of their own and must never be exposed to the client directly. + * + * Same split rationale as workshopDb.ts: keeping the trusted internals out of + * the RPC surface means folding them into a caller doesn't re-run an ownership + * SELECT, and it keeps the guarded/unguarded boundary explicit. + */ +import { db } from "@/db/client"; +import { charts, documents, workspaces } from "@/db/schema"; +import { eq, sql, type SQL } from "drizzle-orm"; + +/** Bump a workspace's updatedAt so recency ordering reflects child edits. */ +export const touchWorkspace = async (workspaceId: string) => { + await db + .update(workspaces) + .set({ updatedAt: new Date() }) + .where(eq(workspaces.id, workspaceId)); +}; + +/** + * SQL scalar for the next position at the bottom of a workspace's unified + * chart+document list, meant to be evaluated *inside* the INSERT that consumes + * it. Folding allocation into the write removes the read-then-write gap the old + * async helper had — where two concurrent creates round-trip a `max(position)` + * read, both see the same value, and insert colliding positions. + * + * SQLite serializes writers, so this is exact there. Under Postgres READ + * COMMITTED two truly-simultaneous inserts can still read the same max before + * either commits; fully serializing would need a row lock, which the create path + * intentionally avoids (see createChartConfigPair / the bun:sqlite async-tx note + * in lensRunQueries). This closes the wide app-level window without a lock. + */ +export const nextWorkspaceItemPositionSql = (workspaceId: string): SQL => + sql`( + select coalesce(max(pos), -1) + 1 + from ( + select ${charts.position} as pos from ${charts} where ${charts.workspaceId} = ${workspaceId} + union all + select ${documents.position} as pos from ${documents} where ${documents.workspaceId} = ${workspaceId} + ) as workspace_positions + )`; diff --git a/workbench/_web/src/lib/queries/lensRunQueries.ts b/workbench/_web/src/lib/queries/lensRunQueries.ts index 32ab6603..463cb435 100644 --- a/workbench/_web/src/lib/queries/lensRunQueries.ts +++ b/workbench/_web/src/lib/queries/lensRunQueries.ts @@ -6,6 +6,7 @@ import type { LensRunSummary, LensRunHeatmaps, LensRunPromptSummary } from "@/ty import type { PatchLensInterventionSpec } from "@/types/patchLens"; import type { LogitLensIntroData } from "@/types/logitLensIntro"; import { and, asc, eq, inArray } from "drizzle-orm"; +import { requireUserId, requireChartOwner, ownedByWorkspace } from "@/lib/auth/ownership"; /** * F1 prompt-history persistence. One row per successful patch-lens lens run, @@ -24,7 +25,6 @@ export type LensRunListItem = Omit; const RETENTION_CAP = 50; export interface CreateLensRunInput { - workspaceId: string; chartId: string; model: string; summary: LensRunSummary; @@ -32,10 +32,14 @@ export interface CreateLensRunInput { } export const createLensRun = async (input: CreateLensRunInput): Promise => { + // INSERT: a run belongs to a chart; the caller must own it. The verified + // workspace is used for the row so a spoofed input.workspaceId can't detach + // the run from its chart's real owner. + const { workspaceId } = await requireChartOwner(input.chartId); const [row] = await db .insert(lensRuns) .values({ - workspaceId: input.workspaceId, + workspaceId, chartId: input.chartId, model: input.model, summary: input.summary, @@ -82,9 +86,14 @@ export const getLensRunsByChart = async ( chartId: string, model?: string, ): Promise => { - // Scope by workspace AND chart so a chart id alone can't read another - // workspace's runs (defense-in-depth; the caller has both from the route). - const conds = [eq(lensRuns.workspaceId, workspaceId), eq(lensRuns.chartId, chartId)]; + const userId = await requireUserId(); + // Scope by owner + workspace AND chart so neither a chart id nor a workspace + // id alone can read another user's runs. + const conds = [ + eq(lensRuns.workspaceId, workspaceId), + eq(lensRuns.chartId, chartId), + ownedByWorkspace(lensRuns.workspaceId, userId), + ]; if (model) conds.push(eq(lensRuns.model, model)); const where = and(...conds); const rows = await db @@ -112,10 +121,11 @@ export const getLensRunHeatmapsByIds = async ( ids: string[], ): Promise<{ id: string; summary: LensRunSummary; data: LensRunHeatmaps }[]> => { if (!ids.length) return []; + const userId = await requireUserId(); const rows = await db .select({ id: lensRuns.id, summary: lensRuns.summary, data: lensRuns.data }) .from(lensRuns) - .where(inArray(lensRuns.id, ids)); + .where(and(inArray(lensRuns.id, ids), ownedByWorkspace(lensRuns.workspaceId, userId))); return rows as { id: string; summary: LensRunSummary; data: LensRunHeatmaps }[]; }; @@ -127,10 +137,17 @@ export const getLensRunHeatmaps = async ( }; export const deleteLensRun = async (workspaceId: string, id: string): Promise => { - // Scope by workspace so a run id alone can't delete another workspace's row. + const userId = await requireUserId(); + // Scope by owner + workspace so a run id alone can't delete another user's row. await db .delete(lensRuns) - .where(and(eq(lensRuns.id, id), eq(lensRuns.workspaceId, workspaceId))); + .where( + and( + eq(lensRuns.id, id), + eq(lensRuns.workspaceId, workspaceId), + ownedByWorkspace(lensRuns.workspaceId, userId), + ), + ); }; /** Clear a chart's history (used by the rail's "Clear" affordance). Scoped by @@ -139,9 +156,16 @@ export const clearLensRunsForChart = async ( workspaceId: string, chartId: string, ): Promise => { + const userId = await requireUserId(); await db .delete(lensRuns) - .where(and(eq(lensRuns.workspaceId, workspaceId), eq(lensRuns.chartId, chartId))); + .where( + and( + eq(lensRuns.workspaceId, workspaceId), + eq(lensRuns.chartId, chartId), + ownedByWorkspace(lensRuns.workspaceId, userId), + ), + ); }; /** @@ -157,10 +181,11 @@ export const updateLensRunIntervention = async ( interventionSummary: LensRunPromptSummary, interventionHeatmap: LogitLensIntroData, ): Promise => { + const userId = await requireUserId(); const [existing] = await db .select({ summary: lensRuns.summary, data: lensRuns.data }) .from(lensRuns) - .where(eq(lensRuns.id, id)); + .where(and(eq(lensRuns.id, id), ownedByWorkspace(lensRuns.workspaceId, userId))); if (!existing) return; const summary: LensRunSummary = { ...(existing.summary as LensRunSummary), @@ -171,5 +196,8 @@ export const updateLensRunIntervention = async ( ...(existing.data as LensRunHeatmaps), interventionResult: interventionHeatmap, }; - await db.update(lensRuns).set({ summary, data }).where(eq(lensRuns.id, id)); + await db + .update(lensRuns) + .set({ summary, data }) + .where(and(eq(lensRuns.id, id), ownedByWorkspace(lensRuns.workspaceId, userId))); }; diff --git a/workbench/_web/src/lib/queries/tutorialChart.ts b/workbench/_web/src/lib/queries/tutorialChart.ts index 93f881cc..30aa6c14 100644 --- a/workbench/_web/src/lib/queries/tutorialChart.ts +++ b/workbench/_web/src/lib/queries/tutorialChart.ts @@ -53,6 +53,10 @@ const replaceChartIds = ( }; export async function pushTutorialChart(workspaceId: string) { + // No pre-flight ownership check: the first DB write below (createLensChartPair) + // guards the workspace via requireWorkspaceOwner before anything is inserted, + // and every create/update helper re-checks — so a non-owner throws before any + // row is written. const createdCharts = []; // Tutorial chart 1 - Translation (Heatmap) diff --git a/workbench/_web/src/lib/queries/viewQueries.ts b/workbench/_web/src/lib/queries/viewQueries.ts index ae62f2d3..d8d4b5d8 100644 --- a/workbench/_web/src/lib/queries/viewQueries.ts +++ b/workbench/_web/src/lib/queries/viewQueries.ts @@ -1,13 +1,15 @@ "use server"; import { db } from "@/db/client"; -import { eq } from "drizzle-orm"; +import { and, eq } from "drizzle-orm"; import { views, charts, type View, type NewView } from "@/db/schema"; import type { ChartType, ChartView } from "@/types/charts"; +import { requireUserId, requireChartOwner, ownedByChart } from "@/lib/auth/ownership"; export const getView = async ( chartId: string, ): Promise<{ view: View; chartType: ChartType } | null> => { + const userId = await requireUserId(); const result = await db .select({ id: views.id, @@ -17,22 +19,38 @@ export const getView = async ( }) .from(views) .leftJoin(charts, eq(views.chartId, charts.id)) - .where(eq(views.chartId, chartId)); + .where(and(eq(views.chartId, chartId), ownedByChart(views.chartId, userId))); if (!result[0]) return null; return { view: result[0], chartType: result[0].chartType as ChartType }; }; export const createView = async (newView: NewView): Promise => { + // INSERT: a view hangs off a chart, so the caller must own that chart. + await requireChartOwner(newView.chartId); const [view] = await db.insert(views).values(newView).returning(); return view; }; export const deleteView = async (id: string): Promise => { - await db.delete(views).where(eq(views.id, id)); + const userId = await requireUserId(); + await db.delete(views).where(and(eq(views.id, id), ownedByChart(views.chartId, userId))); }; -export const updateView = async (id: string, data: ChartView): Promise => { - const [updated] = await db.update(views).set({ data }).where(eq(views.id, id)).returning(); +export const updateView = async (id: string, data: ChartView): Promise => { + const userId = await requireUserId(); + const [updated] = await db + .update(views) + .set({ data }) + .where(and(eq(views.id, id), ownedByChart(views.chartId, userId))) + .returning(); + // Unlike the other guarded writes, a zero-row result here is not treated as a + // forbidden error: the ownedByChart predicate already blocks any cross-user + // write (nothing matched, nothing leaked). It also fires benignly during the + // debounced autosave race — ViewProvider's clear/reset can deleteView between + // the getView read and the 1.5s-debounced updateView, leaving a stale view id + // that matches nothing. Returning undefined lets the provider self-heal + // (deleteView invalidates getView → next autosave createView) instead of + // surfacing a spurious "Error updating view" toast to the owner. return updated; }; diff --git a/workbench/_web/src/lib/queries/workspaceQueries.ts b/workbench/_web/src/lib/queries/workspaceQueries.ts index 378b22a5..c5f44f6f 100644 --- a/workbench/_web/src/lib/queries/workspaceQueries.ts +++ b/workbench/_web/src/lib/queries/workspaceQueries.ts @@ -4,12 +4,16 @@ import { db } from "@/db/client"; import { workspaces, charts, documents } from "@/db/schema"; import { eq, and, sql, desc, isNull } from "drizzle-orm"; import type { ProlificParams } from "@/lib/prolific"; +import { requireUserId, requireWorkspaceOwner } from "@/lib/auth/ownership"; export async function getWorkspaceById(workspaceId: string) { + // Scoped to the caller: the row carries user_id + prolific identifiers, so an + // unowned id must read as "not found" rather than leak another user's data. + const userId = await requireUserId(); const [workspace] = await db .select() .from(workspaces) - .where(eq(workspaces.id, workspaceId)) + .where(and(eq(workspaces.id, workspaceId), eq(workspaces.userId, userId))) .limit(1); return workspace || null; @@ -18,8 +22,8 @@ export async function getWorkspaceById(workspaceId: string) { export async function updateWorkspace( workspaceId: string, updates: { name?: string; public?: boolean }, - userId: string, ) { + const userId = await requireUserId(); const [updatedWorkspace] = await db .update(workspaces) .set(updates) @@ -33,7 +37,8 @@ export async function updateWorkspace( return updatedWorkspace; } -export const getWorkspaces = async (userId: string) => { +export const getWorkspaces = async () => { + const userId = await requireUserId(); const workspaceList = await db .select({ id: workspaces.id, @@ -54,40 +59,23 @@ export const getWorkspaces = async (userId: string) => { return workspaceList; }; -export const deleteWorkspace = async (userId: string, workspaceId: string) => { +export const deleteWorkspace = async (workspaceId: string) => { + const userId = await requireUserId(); await db .delete(workspaces) .where(and(eq(workspaces.id, workspaceId), eq(workspaces.userId, userId))); }; -export const touchWorkspace = async (workspaceId: string) => { - await db - .update(workspaces) - .set({ updatedAt: new Date() }) - .where(eq(workspaces.id, workspaceId)); -}; - -export const getNextWorkspaceItemPosition = async (workspaceId: string): Promise => { - const [chartRows, docRows] = await Promise.all([ - db - .select({ max: sql`max(${charts.position})` }) - .from(charts) - .where(eq(charts.workspaceId, workspaceId)), - db - .select({ max: sql`max(${documents.position})` }) - .from(documents) - .where(eq(documents.workspaceId, workspaceId)), - ]); - const maxPos = Math.max(Number(chartRows[0]?.max ?? -1), Number(docRows[0]?.max ?? -1)); - return maxPos + 1; -}; - export type WorkspaceItemKind = "chart" | "report"; export const reorderWorkspaceItems = async ( workspaceId: string, items: { kind: WorkspaceItemKind; id: string }[], ): Promise => { + // Both the workspace and every item are re-scoped: the workspace by owner, + // and each update by `workspace_id` so a foreign chart/doc id can't be + // slipped into another user's reorder batch. + await requireWorkspaceOwner(workspaceId); await db.transaction(async (tx: typeof db) => { for (let i = 0; i < items.length; i++) { const { kind, id } = items[i]; @@ -106,13 +94,12 @@ export const reorderWorkspaceItems = async ( }); }; -// Wrapped versions for use with server actions/components that use withAuth export const createWorkspace = async ( - userId: string, name: string, workshopId?: string, prolific?: ProlificParams | null, ) => { + const userId = await requireUserId(); const [workspace] = await db .insert(workspaces) .values({ @@ -134,6 +121,7 @@ export const setWorkspaceProlificIfEmpty = async ( workspaceId: string, prolific: ProlificParams, ) => { + await requireWorkspaceOwner(workspaceId); await db .update(workspaces) .set({ prolific })