From 2f59d068bf6d8ae409145b6da0124c19dfb0a083 Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Thu, 16 Jul 2026 20:31:59 +0000 Subject: [PATCH 1/4] security(db): enable RLS on all public tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supabase's PostgREST auto-exposes every table in the `public` schema over HTTP, authorized by the public anon key that ships to every browser. With RLS disabled, anyone holding that key could read or modify all rows (workspaces, prompts, documents, prolific study IDs, and the orphaned `generations` table). This migration closes that hole. - Enables RLS on every base table in `public` via a loop, so current and future orphan tables (e.g. `generations`, dropped from the Drizzle schema when the generation panel was removed) are locked to default-deny rather than silently exposed. - Adds owner-scoped `authenticated` policies to the known application tables, rooted at `workspaces.user_id = auth.uid()::text`; children inherit via `workspace_id` or `chart_id -> charts -> workspace`. - `workshops` and orphan tables get RLS with no policy (server/admin only). - No FORCE, so the `postgres` (Drizzle) and `service_role` (tests) roles keep bypassing — the app and test suite are unaffected. Verified against real Postgres (PGlite): anon sees nothing, an authenticated user sees only their own rows, cross-owner writes are blocked, service_role bypasses, and rowsecurity is on for all tables. Co-Authored-By: Claude Opus 4.8 --- .../migrations/20260716192819_enable_rls.sql | 197 ++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 supabase/migrations/20260716192819_enable_rls.sql diff --git a/supabase/migrations/20260716192819_enable_rls.sql b/supabase/migrations/20260716192819_enable_rls.sql new file mode 100644 index 00000000..498a57b5 --- /dev/null +++ b/supabase/migrations/20260716192819_enable_rls.sql @@ -0,0 +1,197 @@ +-- 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 ─────────────────────────────────────── +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.workspaces w + where w.id = lens_runs.workspace_id + and w.user_id = (select auth.uid())::text + ) + ) + with check ( + exists ( + select 1 from public.workspaces w + where w.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 : owned via chart_id -> charts -> workspace ─────────── +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 + where c.id = chart_config_links.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 = chart_config_links.chart_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 on with NO policy => anon and +-- authenticated are both fully denied over PostgREST; only bypass roles reach it. +alter table public.workshops enable row level security; + +commit; From e19ca18eaca0ef2acc955f4e4b13ce925e16a5ed Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Fri, 17 Jul 2026 19:41:11 +0000 Subject: [PATCH 2/4] security(authz): enforce app-layer ownership on all query RPCs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RLS work runs Drizzle as the postgres role (bypasses RLS), so authorization for the app path lives in the server actions. This converts the query RPCs to derive the caller from the session (requireUserId / requireWorkspaceOwner / requireChartOwner) instead of trusting a client-supplied userId, and closes the gaps found in review: - workspaceQueries: getWorkspaces / updateWorkspace / deleteWorkspace / createWorkspace now derive identity from the session; drop the client-supplied userId args and the dead prop/state plumbing that fed them (WorkspaceList, CreateWorkspaceDialog, AutoWorkspaceCreator, WorkspaceNameEditor, ModelLaunchDialog, LandingPage, page.tsx, workspaceApi hooks). setWorkspaceProlificIfEmpty now requires workspace ownership — previously it had no authorization at all, allowing anyone to stamp Prolific attribution onto any workspace. - setConfig: only data/type are mutable; never rewrite workspaceId, so an owned config can't be relocated into another workspace. - updateView: a zero-row update is a benign no-op (returns undefined), not a ForbiddenError — the ownedByChart predicate already blocks cross-user writes, and the debounced autosave races legitimately with clear/reset, which previously surfaced a spurious error toast. - copyChart: single ownership-scoped full-row fetch (drop the redundant requireChartOwner + re-select round-trip). - pushTutorialChart: drop the redundant pre-flight ownership check (the create helpers already guard before any write). - lensRunQueries: drop the vestigial CreateLensRunInput.workspaceId (the stored value is derived from requireChartOwner). - migration: drop the redundant workshops RLS ALTER (the do-loop already enables it); keep the deny-by-default intent comment. - ownership: note the per-RPC getUser() round-trip as a follow-up. Tests: convert the DB suites to the session-derived signatures (switch identity via setDevUserId for cross-user seeding), rewrite the wrong-user update/delete tests, and add ownership-rejection coverage for getWorkspaces / createWorkspace / setWorkspaceProlificIfEmpty. 76/76 pass. Co-Authored-By: Claude Opus 4.8 --- .../migrations/20260716192819_enable_rls.sql | 6 +- workbench/_web/src/actions/workshop.ts | 2 +- .../components/AutoWorkspaceCreator.tsx | 12 +- .../workbench/components/WorkspaceList.tsx | 12 +- workbench/_web/src/app/workbench/page.tsx | 8 +- .../src/components/CreateWorkspaceDialog.tsx | 7 +- workbench/_web/src/components/LandingPage.tsx | 2 +- .../src/components/WorkspaceNameEditor.tsx | 20 +-- .../components/models/ModelLaunchDialog.tsx | 2 +- .../_web/src/db/__tests__/lensRuns.test.ts | 84 +++++----- .../_web/src/db/__tests__/local-db.test.ts | 150 ++++++++++++++---- .../_web/src/db/__tests__/prolific.test.ts | 25 ++- .../_web/src/db/__tests__/workshops.test.ts | 35 ++-- workbench/_web/src/lib/api/patchLensApi.ts | 1 - workbench/_web/src/lib/api/workspaceApi.ts | 22 +-- workbench/_web/src/lib/auth/devUser.ts | 20 +++ workbench/_web/src/lib/auth/ownership.ts | 128 +++++++++++++++ .../_web/src/lib/queries/chartQueries.ts | 85 +++++++--- .../_web/src/lib/queries/configQueries.ts | 36 ++++- .../_web/src/lib/queries/documentQueries.ts | 41 ++++- workbench/_web/src/lib/queries/internal.ts | 38 +++++ .../_web/src/lib/queries/lensRunQueries.ts | 50 ++++-- .../_web/src/lib/queries/tutorialChart.ts | 4 + workbench/_web/src/lib/queries/viewQueries.ts | 28 +++- .../_web/src/lib/queries/workspaceQueries.ts | 44 ++--- 25 files changed, 631 insertions(+), 231 deletions(-) create mode 100644 workbench/_web/src/lib/auth/devUser.ts create mode 100644 workbench/_web/src/lib/auth/ownership.ts create mode 100644 workbench/_web/src/lib/queries/internal.ts diff --git a/supabase/migrations/20260716192819_enable_rls.sql b/supabase/migrations/20260716192819_enable_rls.sql index 498a57b5..614ec1a0 100644 --- a/supabase/migrations/20260716192819_enable_rls.sql +++ b/supabase/migrations/20260716192819_enable_rls.sql @@ -190,8 +190,8 @@ create policy chart_config_links_owner_all on public.chart_config_links -- ── 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 on with NO policy => anon and --- authenticated are both fully denied over PostgREST; only bypass roles reach it. -alter table public.workshops enable row level security; +-- 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..43da009e 100644 --- a/workbench/_web/src/app/workbench/components/WorkspaceList.tsx +++ b/workbench/_web/src/app/workbench/components/WorkspaceList.tsx @@ -14,10 +14,6 @@ import { useModelsSection } from "@/stores/useModelsSection"; 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 +119,12 @@ function WorkspaceCard({ ); } -export function WorkspaceList({ userId }: WorkspaceListProps) { +export function WorkspaceList() { const deleteWorkspaceMutation = useDeleteWorkspace(); const { data: workspaces, isLoading } = useQuery({ queryKey: ["workspaces"], - queryFn: () => getWorkspaces(userId), + queryFn: () => getWorkspaces(), staleTime: 0, }); @@ -155,7 +151,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 +167,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..612eab92 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,12 +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, + const created = await createLensRun({ chartId: CHART_A, model: "meta-llama/Llama-3.1-8B", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -85,9 +109,7 @@ describe("lens_runs (F1 prompt history)", () => { }); it("never returns the heavy `data` heatmaps from list queries", async () => { - await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" Paris"), heatmaps: heatmaps(" Paris"), @@ -97,9 +119,7 @@ describe("lens_runs (F1 prompt history)", () => { }); it("fetches full heatmaps on demand by id", async () => { - const created = await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + const created = await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -121,9 +141,7 @@ 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, + const created = await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -149,9 +167,7 @@ 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, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(tok), heatmaps: heatmaps(tok), @@ -163,23 +179,17 @@ describe("lens_runs (F1 prompt history)", () => { }); it("scopes history by chart and (optionally) model", async () => { - await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); - await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + await createLensRun({ chartId: CHART_A, model: "m2", summary: summary(" y"), heatmaps: heatmaps(" y"), }); - await createLensRun({ - workspaceId: WS, - chartId: CHART_B, + await createLensRun({ chartId: CHART_B, model: "m1", summary: summary(" z"), heatmaps: heatmaps(" z"), @@ -193,16 +203,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, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); - await createLensRun({ - workspaceId: WS, - chartId: CHART_B, + await createLensRun({ chartId: CHART_B, model: "m1", summary: summary(" y"), heatmaps: heatmaps(" y"), @@ -214,17 +220,13 @@ describe("lens_runs (F1 prompt history)", () => { }); it("deletes a single run by id", async () => { - const a = await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + const a = await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); await new Promise((r) => setTimeout(r, 5)); - await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(" y"), heatmaps: heatmaps(" y"), @@ -240,9 +242,7 @@ describe("lens_runs (F1 prompt history)", () => { const RETENTION_CAP = 50; const total = RETENTION_CAP + 3; for (let i = 0; i < total; i++) { - await createLensRun({ - workspaceId: WS, - chartId: CHART_A, + await createLensRun({ chartId: CHART_A, model: "m1", summary: summary(` t${i}`), heatmaps: heatmaps(), diff --git a/workbench/_web/src/db/__tests__/local-db.test.ts b/workbench/_web/src/db/__tests__/local-db.test.ts index f49a5e97..86f6e290 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..818b4ffa 100644 --- a/workbench/_web/src/lib/api/workspaceApi.ts +++ b/workbench/_web/src/lib/api/workspaceApi.ts @@ -14,9 +14,9 @@ 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: () => { @@ -33,8 +33,8 @@ 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"] }); @@ -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..380e5f6c --- /dev/null +++ b/workbench/_web/src/lib/auth/ownership.ts @@ -0,0 +1,128 @@ +/** + * 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") { + // 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..ede5de93 100644 --- a/workbench/_web/src/lib/queries/chartQueries.ts +++ b/workbench/_web/src/lib/queries/chartQueries.ts @@ -2,55 +2,84 @@ 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, getNextWorkspaceItemPosition } 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 +102,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. @@ -121,7 +153,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 +166,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 +186,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 +212,7 @@ 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,11 +266,20 @@ 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 const [originalLink] = await db diff --git a/workbench/_web/src/lib/queries/configQueries.ts b/workbench/_web/src/lib/queries/configQueries.ts index c85e5187..ca5ffd85 100644 --- a/workbench/_web/src/lib/queries/configQueries.ts +++ b/workbench/_web/src/lib/queries/configQueries.ts @@ -1,30 +1,56 @@ "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, +} 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. + await requireChartOwner(chartId); 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..9f976e74 100644 --- a/workbench/_web/src/lib/queries/documentQueries.ts +++ b/workbench/_web/src/lib/queries/documentQueries.ts @@ -2,21 +2,34 @@ 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, getNextWorkspaceItemPosition } 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 +39,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 +115,18 @@ 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,6 +176,8 @@ 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); @@ -173,5 +195,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..a61dabfe --- /dev/null +++ b/workbench/_web/src/lib/queries/internal.ts @@ -0,0 +1,38 @@ +/** + * 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 } 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)); +}; + +/** Next position at the bottom of a workspace's unified chart+document list. */ +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; +}; 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 }) From c0af58f1560997feebefe855f165059de7bd3cd1 Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Fri, 17 Jul 2026 19:43:28 +0000 Subject: [PATCH 3/4] style: prettier-format the ownership authz changes Formatting-only follow-up to satisfy the Run Prettier CI gate. Co-Authored-By: Claude Opus 4.8 --- .../_web/src/db/__tests__/lensRuns.test.ts | 39 ++++++++++++------- .../_web/src/db/__tests__/local-db.test.ts | 6 +-- .../_web/src/lib/queries/chartQueries.ts | 23 +++++++++-- .../_web/src/lib/queries/configQueries.ts | 5 ++- .../_web/src/lib/queries/documentQueries.ts | 10 ++++- 5 files changed, 60 insertions(+), 23 deletions(-) diff --git a/workbench/_web/src/db/__tests__/lensRuns.test.ts b/workbench/_web/src/db/__tests__/lensRuns.test.ts index 612eab92..462b7b78 100644 --- a/workbench/_web/src/db/__tests__/lensRuns.test.ts +++ b/workbench/_web/src/db/__tests__/lensRuns.test.ts @@ -93,7 +93,8 @@ describe("lens_runs (F1 prompt history)", () => { }); it("creates a run and reads back the compact summary payload intact", async () => { - const created = await createLensRun({ chartId: CHART_A, + const created = await createLensRun({ + chartId: CHART_A, model: "meta-llama/Llama-3.1-8B", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -109,7 +110,8 @@ describe("lens_runs (F1 prompt history)", () => { }); it("never returns the heavy `data` heatmaps from list queries", async () => { - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" Paris"), heatmaps: heatmaps(" Paris"), @@ -119,7 +121,8 @@ describe("lens_runs (F1 prompt history)", () => { }); it("fetches full heatmaps on demand by id", async () => { - const created = await createLensRun({ chartId: CHART_A, + const created = await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -141,7 +144,8 @@ describe("lens_runs (F1 prompt history)", () => { }); it("attaches a patch to an existing run via updateLensRunIntervention", async () => { - const created = await createLensRun({ chartId: CHART_A, + const created = await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" Paris", " Rome"), heatmaps: heatmaps(" Paris", " Rome"), @@ -167,7 +171,8 @@ 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({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(tok), heatmaps: heatmaps(tok), @@ -179,17 +184,20 @@ describe("lens_runs (F1 prompt history)", () => { }); it("scopes history by chart and (optionally) model", async () => { - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m2", summary: summary(" y"), heatmaps: heatmaps(" y"), }); - await createLensRun({ chartId: CHART_B, + await createLensRun({ + chartId: CHART_B, model: "m1", summary: summary(" z"), heatmaps: heatmaps(" z"), @@ -203,12 +211,14 @@ describe("lens_runs (F1 prompt history)", () => { }); it("clears a chart's history without touching another chart", async () => { - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); - await createLensRun({ chartId: CHART_B, + await createLensRun({ + chartId: CHART_B, model: "m1", summary: summary(" y"), heatmaps: heatmaps(" y"), @@ -220,13 +230,15 @@ describe("lens_runs (F1 prompt history)", () => { }); it("deletes a single run by id", async () => { - const a = await createLensRun({ chartId: CHART_A, + const a = await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" x"), heatmaps: heatmaps(" x"), }); await new Promise((r) => setTimeout(r, 5)); - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(" y"), heatmaps: heatmaps(" y"), @@ -242,7 +254,8 @@ describe("lens_runs (F1 prompt history)", () => { const RETENTION_CAP = 50; const total = RETENTION_CAP + 3; for (let i = 0; i < total; i++) { - await createLensRun({ chartId: CHART_A, + await createLensRun({ + chartId: CHART_A, model: "m1", summary: summary(` t${i}`), heatmaps: heatmaps(), diff --git a/workbench/_web/src/db/__tests__/local-db.test.ts b/workbench/_web/src/db/__tests__/local-db.test.ts index 86f6e290..cce37b9d 100644 --- a/workbench/_web/src/db/__tests__/local-db.test.ts +++ b/workbench/_web/src/db/__tests__/local-db.test.ts @@ -647,9 +647,9 @@ describe("Ownership enforcement (scoped queries reject other users)", () => { 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); + 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 () => { diff --git a/workbench/_web/src/lib/queries/chartQueries.ts b/workbench/_web/src/lib/queries/chartQueries.ts index ede5de93..ff1abe4f 100644 --- a/workbench/_web/src/lib/queries/chartQueries.ts +++ b/workbench/_web/src/lib/queries/chartQueries.ts @@ -2,7 +2,15 @@ import type { ChartData, ChartMetadata, ChartView, ChartType, ToolType } from "@/types/charts"; import { db } from "@/db/client"; -import { charts, configs, chartConfigLinks, workspaces, 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"; @@ -78,7 +86,10 @@ export const getConfigForChart = async (chartId: string): Promise .from(configs) .innerJoin(chartConfigLinks, eq(configs.id, chartConfigLinks.configId)) .where( - and(eq(chartConfigLinks.chartId, chartId), ownedByWorkspace(configs.workspaceId, userId)), + and( + eq(chartConfigLinks.chartId, chartId), + ownedByWorkspace(configs.workspaceId, userId), + ), ) .limit(1); if (rows.length === 0) return null; @@ -212,7 +223,9 @@ export const getChartsMetadata = async (workspaceId: string): Promise => { .from(configs) .innerJoin(chartConfigLinks, eq(configs.id, chartConfigLinks.configId)) .where( - and(eq(chartConfigLinks.chartId, chartId), ownedByWorkspace(configs.workspaceId, userId)), + 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 diff --git a/workbench/_web/src/lib/queries/documentQueries.ts b/workbench/_web/src/lib/queries/documentQueries.ts index 9f976e74..923e53a3 100644 --- a/workbench/_web/src/lib/queries/documentQueries.ts +++ b/workbench/_web/src/lib/queries/documentQueries.ts @@ -28,7 +28,10 @@ export const getDocumentByWorkspaceId = async (workspaceId: string): Promise Date: Fri, 17 Jul 2026 23:21:42 +0000 Subject: [PATCH 4/4] security(authz): address review follow-ups (link ownership, RLS, fail-closed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening from the PR #131 review pass: - ownership: requireUserId() fails closed in production — a build with NEXT_PUBLIC_DISABLE_AUTH=true throws instead of handing every RPC the synthetic dev identity. - addChartConfigLink: verify the config lives in the chart's owned workspace (chart ownership alone didn't establish config ownership, letting a caller link — and via copyChart, read — another tenant's config). copyChart's config lookup is likewise scoped to the chart's workspace and refuses a cross-workspace link. - RLS: tighten the lens_runs policy so chart_id and workspace_id must resolve to the same owned workspace, and the chart_config_links policy so the config shares the chart's owned workspace (defense-in-depth behind the app-layer checks above). - internal.ts: allocate the next list position inside the INSERT via a portable subquery instead of a separate read-then-write, removing the app-level window where concurrent creates collide on position. - WorkspaceList / workspaceApi: use the centralized queryKeys.workspaces.all instead of a hardcoded ["workspaces"] key. Tests: add lens-run cross-user coverage (createLensRun rejects another user's chart; list / heatmap / patch / clear / delete can't reach that chart's runs). 79/79 DB tests pass. Co-Authored-By: Claude Opus 4.8 --- .../migrations/20260716192819_enable_rls.sql | 26 ++++++-- .../workbench/components/WorkspaceList.tsx | 3 +- .../_web/src/db/__tests__/lensRuns.test.ts | 59 +++++++++++++++++++ workbench/_web/src/lib/api/workspaceApi.ts | 4 +- workbench/_web/src/lib/auth/ownership.ts | 6 ++ .../_web/src/lib/queries/chartQueries.ts | 27 ++++++--- .../_web/src/lib/queries/configQueries.ts | 14 ++++- .../_web/src/lib/queries/documentQueries.ts | 5 +- workbench/_web/src/lib/queries/internal.ts | 39 +++++++----- 9 files changed, 146 insertions(+), 37 deletions(-) diff --git a/supabase/migrations/20260716192819_enable_rls.sql b/supabase/migrations/20260716192819_enable_rls.sql index 614ec1a0..04b34cc6 100644 --- a/supabase/migrations/20260716192819_enable_rls.sql +++ b/supabase/migrations/20260716192819_enable_rls.sql @@ -124,22 +124,29 @@ create policy documents_owner_all on public.documents ) ); --- ── lens_runs : owned via workspace_id ─────────────────────────────────────── +-- ── 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.workspaces w - where w.id = lens_runs.workspace_id + 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.workspaces w - where w.id = lens_runs.workspace_id + 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 ) ); @@ -166,7 +173,10 @@ create policy views_owner_all on public.views ) ); --- ── chart_config_links : owned via chart_id -> charts -> workspace ─────────── +-- ── 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 @@ -175,7 +185,9 @@ create policy chart_config_links_owner_all on public.chart_config_links 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 ) ) @@ -183,7 +195,9 @@ create policy chart_config_links_owner_all on public.chart_config_links 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 ) ); diff --git a/workbench/_web/src/app/workbench/components/WorkspaceList.tsx b/workbench/_web/src/app/workbench/components/WorkspaceList.tsx index 43da009e..dd22a010 100644 --- a/workbench/_web/src/app/workbench/components/WorkspaceList.tsx +++ b/workbench/_web/src/app/workbench/components/WorkspaceList.tsx @@ -10,6 +10,7 @@ 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 @@ -123,7 +124,7 @@ export function WorkspaceList() { const deleteWorkspaceMutation = useDeleteWorkspace(); const { data: workspaces, isLoading } = useQuery({ - queryKey: ["workspaces"], + queryKey: queryKeys.workspaces.all, queryFn: () => getWorkspaces(), staleTime: 0, }); diff --git a/workbench/_web/src/db/__tests__/lensRuns.test.ts b/workbench/_web/src/db/__tests__/lensRuns.test.ts index 462b7b78..984c507f 100644 --- a/workbench/_web/src/db/__tests__/lensRuns.test.ts +++ b/workbench/_web/src/db/__tests__/lensRuns.test.ts @@ -274,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/lib/api/workspaceApi.ts b/workbench/_web/src/lib/api/workspaceApi.ts index 818b4ffa..e3cd2c31 100644 --- a/workbench/_web/src/lib/api/workspaceApi.ts +++ b/workbench/_web/src/lib/api/workspaceApi.ts @@ -20,7 +20,7 @@ export const useCreateWorkspace = () => { return newWorkspace; }, onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["workspaces"] }); + queryClient.invalidateQueries({ queryKey: queryKeys.workspaces.all }); console.log("Successfully created workspace"); }, onError: (error) => { @@ -37,7 +37,7 @@ export const useDeleteWorkspace = () => { await deleteWorkspace(workspaceId); }, onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["workspaces"] }); + queryClient.invalidateQueries({ queryKey: queryKeys.workspaces.all }); console.log("Successfully deleted workspace"); }, onError: (error) => { diff --git a/workbench/_web/src/lib/auth/ownership.ts b/workbench/_web/src/lib/auth/ownership.ts index 380e5f6c..bb9e16fc 100644 --- a/workbench/_web/src/lib/auth/ownership.ts +++ b/workbench/_web/src/lib/auth/ownership.ts @@ -43,6 +43,12 @@ export class ForbiddenError extends Error { */ 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(); diff --git a/workbench/_web/src/lib/queries/chartQueries.ts b/workbench/_web/src/lib/queries/chartQueries.ts index ff1abe4f..f5acf72b 100644 --- a/workbench/_web/src/lib/queries/chartQueries.ts +++ b/workbench/_web/src/lib/queries/chartQueries.ts @@ -17,7 +17,7 @@ import { PatchingConfig } from "@/types/patching"; import { ActivationPatchingConfigData } from "@/types/activationPatching"; import { PatchLensChartData } from "@/types/patchLens"; import { eq, and, asc, desc, sql } from "drizzle-orm"; -import { touchWorkspace, getNextWorkspaceItemPosition } from "@/lib/queries/internal"; +import { touchWorkspace, nextWorkspaceItemPositionSql } from "@/lib/queries/internal"; import { requireUserId, requireWorkspaceOwner, @@ -123,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) @@ -296,7 +299,10 @@ export const copyChart = async (chartId: string): Promise => { } 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) @@ -305,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. @@ -316,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({ @@ -325,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 c837d485..2e91b81e 100644 --- a/workbench/_web/src/lib/queries/configQueries.ts +++ b/workbench/_web/src/lib/queries/configQueries.ts @@ -8,6 +8,7 @@ import { requireWorkspaceOwner, requireChartOwner, ownedByWorkspace, + ForbiddenError, } from "@/lib/auth/ownership"; export const setConfig = async (configId: string, config: NewConfig): Promise => { @@ -35,8 +36,17 @@ export const deleteConfig = async (configId: string): Promise => { }; export const addChartConfigLink = async (configId: string, chartId: string): Promise => { - // INSERT: linking a config to a chart — the caller must own the chart. - await requireChartOwner(chartId); + // 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 }); }; diff --git a/workbench/_web/src/lib/queries/documentQueries.ts b/workbench/_web/src/lib/queries/documentQueries.ts index 923e53a3..340dfdc6 100644 --- a/workbench/_web/src/lib/queries/documentQueries.ts +++ b/workbench/_web/src/lib/queries/documentQueries.ts @@ -4,7 +4,7 @@ import { db } from "@/db/client"; import { documents, Document } from "@/db/schema"; import { and, asc, eq } from "drizzle-orm"; import { SerializedEditorState } from "lexical"; -import { touchWorkspace, getNextWorkspaceItemPosition } from "@/lib/queries/internal"; +import { touchWorkspace, nextWorkspaceItemPositionSql } from "@/lib/queries/internal"; import { requireUserId, requireWorkspaceOwner, @@ -185,14 +185,13 @@ 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(); diff --git a/workbench/_web/src/lib/queries/internal.ts b/workbench/_web/src/lib/queries/internal.ts index a61dabfe..45b01c05 100644 --- a/workbench/_web/src/lib/queries/internal.ts +++ b/workbench/_web/src/lib/queries/internal.ts @@ -11,7 +11,7 @@ */ import { db } from "@/db/client"; import { charts, documents, workspaces } from "@/db/schema"; -import { eq, sql } from "drizzle-orm"; +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) => { @@ -21,18 +21,25 @@ export const touchWorkspace = async (workspaceId: string) => { .where(eq(workspaces.id, workspaceId)); }; -/** Next position at the bottom of a workspace's unified chart+document list. */ -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; -}; +/** + * 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 + )`;