From 7ff750ad5e40e0b794cc8e496e7784f24b33a3ba Mon Sep 17 00:00:00 2001 From: Kartikeya <176560021+karti-ai@users.noreply.github.com> Date: Sat, 27 Jun 2026 16:02:40 -0700 Subject: [PATCH] harden: address adversarial review of Pods CRUD Backend: atomic create with insert-retry-on-duplicate-key (removes slug TOCTOU race + wrong 400s), resilient per-index creation so a unique-index failure can't silently drop the constraint or block seeding, idempotent race-safe seeding via $setOnInsert, type validation + size caps on all pod fields/members, removeMember no-ops without a write, /api/outcome guarded. Frontend: per-pod busy state (one mutation no longer disables every card), joined pod derived from the list (can't go stale), create keeps inputs on error. Co-Authored-By: Claude Opus 4.8 --- backend/src/memory/db.ts | 26 +++-- backend/src/pods/store.ts | 114 +++++++++++++++------- backend/src/server.ts | 12 ++- frontend/src/App.tsx | 60 ++++++++---- frontend/src/components/CreatePodForm.tsx | 29 +++--- 5 files changed, 163 insertions(+), 78 deletions(-) diff --git a/backend/src/memory/db.ts b/backend/src/memory/db.ts index 4513a18..629d08c 100644 --- a/backend/src/memory/db.ts +++ b/backend/src/memory/db.ts @@ -48,13 +48,23 @@ export async function initMemory(): Promise { const db = await getDb(); await db.command({ ping: 1 }); const c = await collections(); - await Promise.all([ - c.pods.createIndex({ id: 1 }, { unique: true }), - c.observations.createIndex({ podId: 1, observedAt: -1 }), - c.observations.createIndex({ engineerId: 1 }), - c.collisions.createIndex({ podId: 1, detectedAt: -1 }), - c.interventions.createIndex({ collisionId: 1 }), - c.outcomes.createIndex({ interventionId: 1 }), - ]); + // Create each index independently so one failure (e.g. the unique pods index + // failing on pre-existing duplicate ids) doesn't abort the others or block + // seeding. Failures are logged loudly rather than silently swallowed. + const indexes: Array<[string, () => Promise]> = [ + ['pods.id (unique)', () => c.pods.createIndex({ id: 1 }, { unique: true })], + ['observations.podId', () => c.observations.createIndex({ podId: 1, observedAt: -1 })], + ['observations.engineerId', () => c.observations.createIndex({ engineerId: 1 })], + ['collisions.podId', () => c.collisions.createIndex({ podId: 1, detectedAt: -1 })], + ['interventions.collisionId', () => c.interventions.createIndex({ collisionId: 1 })], + ['outcomes.interventionId', () => c.outcomes.createIndex({ interventionId: 1 })], + ]; + for (const [name, make] of indexes) { + try { + await make(); + } catch (err) { + console.error(`[memory] index "${name}" failed: ${(err as Error).message}`); + } + } console.log(`[memory] mongo connected -> ${db.databaseName}`); } diff --git a/backend/src/pods/store.ts b/backend/src/pods/store.ts index 4694c48..27c9728 100644 --- a/backend/src/pods/store.ts +++ b/backend/src/pods/store.ts @@ -3,6 +3,12 @@ import { collections } from '../memory/db.js'; const NO_ID = { projection: { _id: 0 } } as const; +const MAX_NAME = 120; +const MAX_REPO = 140; +const MAX_DESCRIPTION = 2000; +const MAX_MEMBERS = 50; +const MAX_MEMBER_LEN = 80; + function slugify(name: string): string { return name .toLowerCase() @@ -11,22 +17,43 @@ function slugify(name: string): string { .replace(/^-+|-+$/g, ''); } -/** Trim, drop empties, de-dupe case-insensitively while preserving order. */ +/** Validate + normalize a string field. Throws (-> 400) on type/length errors. */ +function str(value: unknown, field: string, max: number, required = false): string | undefined { + if (value === undefined || value === null) { + if (required) throw new Error(`${field} is required`); + return undefined; + } + if (typeof value !== 'string') throw new Error(`${field} must be a string`); + const trimmed = value.trim(); + if (required && !trimmed) throw new Error(`${field} is required`); + if (trimmed.length > max) throw new Error(`${field} too long (max ${max})`); + return trimmed; +} + +/** Validate, trim, length-cap, de-dupe (case-insensitive), and limit count. */ function cleanMembers(members: unknown): string[] { - if (!Array.isArray(members)) return []; + if (members === undefined || members === null) return []; + if (!Array.isArray(members)) throw new Error('members must be an array'); const seen = new Set(); const out: string[] = []; for (const raw of members) { - const name = String(raw).trim(); + if (typeof raw !== 'string') throw new Error('member names must be strings'); + const name = raw.trim(); + if (name.length > MAX_MEMBER_LEN) throw new Error(`member name too long (max ${MAX_MEMBER_LEN})`); const key = name.toLowerCase(); if (name && !seen.has(key)) { seen.add(key); out.push(name); } } + if (out.length > MAX_MEMBERS) throw new Error(`too many members (max ${MAX_MEMBERS})`); return out; } +function isDuplicateKey(err: unknown): boolean { + return (err as { code?: number })?.code === 11000; +} + function now(): string { return new Date().toISOString(); } @@ -41,42 +68,49 @@ export async function getPod(id: string): Promise { return c.pods.findOne({ id }, NO_ID); } -async function uniqueSlug(base: string): Promise { - const c = await collections(); - const root = base || 'pod'; - let slug = root; - let n = 2; - while (await c.pods.findOne({ id: slug })) slug = `${root}-${n++}`; - return slug; -} - export async function createPod(input: PodInput): Promise { - const name = (input.name ?? '').trim(); - if (!name) throw new Error('name is required'); + const name = str(input.name, 'name', MAX_NAME, true)!; + const repo = str(input.repo, 'repo', MAX_REPO) ?? ''; + const description = str(input.description, 'description', MAX_DESCRIPTION); + const members = cleanMembers(input.members); const c = await collections(); const ts = now(); - const pod: Pod = { - id: await uniqueSlug(slugify(name)), - name, - repo: (input.repo ?? '').trim(), - description: input.description?.trim() || undefined, - members: cleanMembers(input.members), - createdAt: ts, - updatedAt: ts, - }; - await c.pods.insertOne({ ...pod }); - return pod; + const base = slugify(name) || 'pod'; + + // Atomic create: let the unique index arbitrate slug collisions. On a + // duplicate-key error, bump the suffix and retry — no check-then-insert race. + for (let suffix = 1; suffix <= MAX_MEMBERS + 50; suffix++) { + const pod: Pod = { + id: suffix === 1 ? base : `${base}-${suffix}`, + name, + repo, + description: description || undefined, + members, + createdAt: ts, + updatedAt: ts, + }; + try { + await c.pods.insertOne({ ...pod }); + return pod; + } catch (err) { + if (isDuplicateKey(err)) continue; + throw err; + } + } + throw new Error('could not allocate a unique pod id'); } export async function updatePod(id: string, patch: PodInput): Promise { const set: Partial = { updatedAt: now() }; if (patch.name !== undefined) { - const name = patch.name.trim(); + const name = str(patch.name, 'name', MAX_NAME); if (!name) throw new Error('name cannot be empty'); set.name = name; } - if (patch.repo !== undefined) set.repo = patch.repo.trim(); - if (patch.description !== undefined) set.description = patch.description.trim() || undefined; + if (patch.repo !== undefined) set.repo = str(patch.repo, 'repo', MAX_REPO) ?? ''; + if (patch.description !== undefined) { + set.description = str(patch.description, 'description', MAX_DESCRIPTION) || undefined; + } if (patch.members !== undefined) set.members = cleanMembers(patch.members); const c = await collections(); const updated = await c.pods.findOneAndUpdate( @@ -103,26 +137,32 @@ async function setMembers(id: string, members: string[]): Promise { return updated ?? null; } -export async function addMember(id: string, rawName: string): Promise { +export async function addMember(id: string, rawName: unknown): Promise { + if (typeof rawName !== 'string') throw new Error('member name must be a string'); const name = rawName.trim(); if (!name) throw new Error('member name is required'); + if (name.length > MAX_MEMBER_LEN) throw new Error(`member name too long (max ${MAX_MEMBER_LEN})`); const pod = await getPod(id); if (!pod) return null; if (pod.members.some((m) => m.toLowerCase() === name.toLowerCase())) return pod; + if (pod.members.length >= MAX_MEMBERS) throw new Error(`too many members (max ${MAX_MEMBERS})`); return setMembers(id, [...pod.members, name]); } export async function removeMember(id: string, rawName: string): Promise { - const name = rawName.trim().toLowerCase(); + const name = String(rawName).trim().toLowerCase(); const pod = await getPod(id); if (!pod) return null; - return setMembers( - id, - pod.members.filter((m) => m.toLowerCase() !== name), - ); + const filtered = pod.members.filter((m) => m.toLowerCase() !== name); + if (filtered.length === pod.members.length) return pod; // no-op: don't write / bump updatedAt + return setMembers(id, filtered); } -/** Insert the default pods once, if the collection is empty. */ +/** + * Seed the default pods into a fresh DB. Idempotent and race-safe: gated on an + * empty collection, and uses per-doc upserts so concurrent startups can't + * create duplicates. Won't resurrect a default a user later deletes. + */ export async function seedDefaultPods(): Promise { const c = await collections(); if ((await c.pods.estimatedDocumentCount()) > 0) return; @@ -156,6 +196,8 @@ export async function seedDefaultPods(): Promise { updatedAt: ts, }, ]; - await c.pods.insertMany(defaults.map((p) => ({ ...p }))); + await Promise.all( + defaults.map((p) => c.pods.updateOne({ id: p.id }, { $setOnInsert: p }, { upsert: true })), + ); console.log('[pods] seeded default pods'); } diff --git a/backend/src/server.ts b/backend/src/server.ts index ab9c8e8..dd91021 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -51,8 +51,16 @@ app.post('/api/sync-pr', async (req, res) => { // Outcome ACK -> closes the continual-learning policy loop. app.post('/api/outcome', async (req, res) => { - await recordOutcome(req.body as InterventionOutcome); - res.json({ ok: true }); + try { + const o = (req.body ?? {}) as Partial; + if (typeof o.interventionId !== 'string' || !o.interventionId) { + return res.status(400).json({ error: 'interventionId is required' }); + } + await recordOutcome(req.body as InterventionOutcome); + res.json({ ok: true }); + } catch (e) { + res.status(500).json({ error: (e as Error).message }); + } }); // Memory counts — quick way to confirm Mongo persistence is working. diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 8f2cada..d95ce92 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -10,15 +10,17 @@ import { PodView } from './components/PodView.js'; export default function App() { const [pods, setPods] = useState([]); const [loading, setLoading] = useState(true); - const [busy, setBusy] = useState(false); + const [pending, setPending] = useState>(new Set()); const [error, setError] = useState(null); - // join state - const [joinedPod, setJoinedPod] = useState(null); + // join state — keep the id and derive the pod so it never goes stale + const [joinedPodId, setJoinedPodId] = useState(null); const [member, setMember] = useState(''); const [devMode, setDevMode] = useState(false); const [room, setRoom] = useState(null); + const joinedPod = joinedPodId ? (pods.find((p) => p.id === joinedPodId) ?? null) : null; + async function refresh() { setLoading(true); try { @@ -35,40 +37,58 @@ export default function App() { void refresh(); }, []); - /** Run a mutation, reflect the result in local state, surface errors. */ - async function mutate(fn: () => Promise) { - setBusy(true); + const startPending = (key: string) => setPending((s) => new Set(s).add(key)); + const endPending = (key: string) => + setPending((s) => { + const n = new Set(s); + n.delete(key); + return n; + }); + + /** Run a mutation keyed by pod id (or 'new'); only that card shows busy. */ + async function run(key: string, fn: () => Promise) { + startPending(key); setError(null); try { await fn(); } catch (e) { setError((e as Error).message); } finally { - setBusy(false); + endPending(key); } } const upsert = (p: Pod) => setPods((cur) => cur.map((x) => (x.id === p.id ? p : x))); - const handleCreate = (input: PodInput) => - mutate(async () => { + // create rethrows so CreatePodForm can keep the user's input on failure + async function handleCreate(input: PodInput): Promise { + startPending('new'); + setError(null); + try { const created = await api.createPod(input); setPods((cur) => [...cur, created]); - }); + } catch (e) { + setError((e as Error).message); + throw e; + } finally { + endPending('new'); + } + } + const handleUpdate = (id: string, patch: PodInput) => - mutate(async () => upsert(await api.updatePod(id, patch))); + run(id, async () => upsert(await api.updatePod(id, patch))); const handleDelete = (id: string) => - mutate(async () => { + run(id, async () => { await api.deletePod(id); setPods((cur) => cur.filter((x) => x.id !== id)); }); const handleAddMember = (id: string, name: string) => - mutate(async () => upsert(await api.addMember(id, name))); + run(id, async () => upsert(await api.addMember(id, name))); const handleRemoveMember = (id: string, name: string) => - mutate(async () => upsert(await api.removeMember(id, name))); + run(id, async () => upsert(await api.removeMember(id, name))); async function handleJoin(pod: Pod, who: string) { - setBusy(true); + startPending(pod.id); setError(null); try { const identity = `${who}-${Math.random().toString(36).slice(2, 7)}`; @@ -76,18 +96,18 @@ export default function App() { setRoom(result.room); setDevMode(result.mode === 'dev'); setMember(who); - setJoinedPod(pod); + setJoinedPodId(pod.id); } catch (e) { setError((e as Error).message); } finally { - setBusy(false); + endPending(pod.id); } } function handleLeave() { room?.disconnect(); setRoom(null); - setJoinedPod(null); + setJoinedPodId(null); } return ( @@ -133,7 +153,7 @@ export default function App() { ))} - + )} diff --git a/frontend/src/components/CreatePodForm.tsx b/frontend/src/components/CreatePodForm.tsx index de142ac..8ef45d8 100644 --- a/frontend/src/components/CreatePodForm.tsx +++ b/frontend/src/components/CreatePodForm.tsx @@ -6,7 +6,7 @@ export function CreatePodForm({ onCreate, }: { busy: boolean; - onCreate: (input: PodInput) => void; + onCreate: (input: PodInput) => Promise; }) { const [open, setOpen] = useState(false); const [name, setName] = useState(''); @@ -14,18 +14,23 @@ export function CreatePodForm({ const [description, setDescription] = useState(''); const [firstMember, setFirstMember] = useState(''); - function submit() { + async function submit() { if (!name.trim()) return; - onCreate({ - name: name.trim(), - repo: repo.trim(), - description: description.trim(), - members: firstMember.trim() ? [firstMember.trim()] : [], - }); - setName(''); - setDescription(''); - setFirstMember(''); - setOpen(false); + try { + await onCreate({ + name: name.trim(), + repo: repo.trim(), + description: description.trim(), + members: firstMember.trim() ? [firstMember.trim()] : [], + }); + // only clear + close on success; on error keep the user's input + setName(''); + setDescription(''); + setFirstMember(''); + setOpen(false); + } catch { + /* error surfaced by parent; keep inputs so nothing is lost */ + } } if (!open) {