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 <noreply@anthropic.com>
This commit is contained in:
Kartikeya
2026-06-27 16:02:40 -07:00
parent 4ddc59be08
commit 7ff750ad5e
5 changed files with 163 additions and 78 deletions
+18 -8
View File
@@ -48,13 +48,23 @@ export async function initMemory(): Promise<void> {
const db = await getDb(); const db = await getDb();
await db.command({ ping: 1 }); await db.command({ ping: 1 });
const c = await collections(); const c = await collections();
await Promise.all([ // Create each index independently so one failure (e.g. the unique pods index
c.pods.createIndex({ id: 1 }, { unique: true }), // failing on pre-existing duplicate ids) doesn't abort the others or block
c.observations.createIndex({ podId: 1, observedAt: -1 }), // seeding. Failures are logged loudly rather than silently swallowed.
c.observations.createIndex({ engineerId: 1 }), const indexes: Array<[string, () => Promise<unknown>]> = [
c.collisions.createIndex({ podId: 1, detectedAt: -1 }), ['pods.id (unique)', () => c.pods.createIndex({ id: 1 }, { unique: true })],
c.interventions.createIndex({ collisionId: 1 }), ['observations.podId', () => c.observations.createIndex({ podId: 1, observedAt: -1 })],
c.outcomes.createIndex({ interventionId: 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}`); console.log(`[memory] mongo connected -> ${db.databaseName}`);
} }
+71 -29
View File
@@ -3,6 +3,12 @@ import { collections } from '../memory/db.js';
const NO_ID = { projection: { _id: 0 } } as const; 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 { function slugify(name: string): string {
return name return name
.toLowerCase() .toLowerCase()
@@ -11,22 +17,43 @@ function slugify(name: string): string {
.replace(/^-+|-+$/g, ''); .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[] { 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<string>(); const seen = new Set<string>();
const out: string[] = []; const out: string[] = [];
for (const raw of members) { 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(); const key = name.toLowerCase();
if (name && !seen.has(key)) { if (name && !seen.has(key)) {
seen.add(key); seen.add(key);
out.push(name); out.push(name);
} }
} }
if (out.length > MAX_MEMBERS) throw new Error(`too many members (max ${MAX_MEMBERS})`);
return out; return out;
} }
function isDuplicateKey(err: unknown): boolean {
return (err as { code?: number })?.code === 11000;
}
function now(): string { function now(): string {
return new Date().toISOString(); return new Date().toISOString();
} }
@@ -41,42 +68,49 @@ export async function getPod(id: string): Promise<Pod | null> {
return c.pods.findOne({ id }, NO_ID); return c.pods.findOne({ id }, NO_ID);
} }
async function uniqueSlug(base: string): Promise<string> {
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<Pod> { export async function createPod(input: PodInput): Promise<Pod> {
const name = (input.name ?? '').trim(); const name = str(input.name, 'name', MAX_NAME, true)!;
if (!name) throw new Error('name is required'); 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 c = await collections();
const ts = now(); const ts = now();
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 = { const pod: Pod = {
id: await uniqueSlug(slugify(name)), id: suffix === 1 ? base : `${base}-${suffix}`,
name, name,
repo: (input.repo ?? '').trim(), repo,
description: input.description?.trim() || undefined, description: description || undefined,
members: cleanMembers(input.members), members,
createdAt: ts, createdAt: ts,
updatedAt: ts, updatedAt: ts,
}; };
try {
await c.pods.insertOne({ ...pod }); await c.pods.insertOne({ ...pod });
return 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<Pod | null> { export async function updatePod(id: string, patch: PodInput): Promise<Pod | null> {
const set: Partial<Pod> = { updatedAt: now() }; const set: Partial<Pod> = { updatedAt: now() };
if (patch.name !== undefined) { 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'); if (!name) throw new Error('name cannot be empty');
set.name = name; set.name = name;
} }
if (patch.repo !== undefined) set.repo = patch.repo.trim(); if (patch.repo !== undefined) set.repo = str(patch.repo, 'repo', MAX_REPO) ?? '';
if (patch.description !== undefined) set.description = patch.description.trim() || undefined; if (patch.description !== undefined) {
set.description = str(patch.description, 'description', MAX_DESCRIPTION) || undefined;
}
if (patch.members !== undefined) set.members = cleanMembers(patch.members); if (patch.members !== undefined) set.members = cleanMembers(patch.members);
const c = await collections(); const c = await collections();
const updated = await c.pods.findOneAndUpdate( const updated = await c.pods.findOneAndUpdate(
@@ -103,26 +137,32 @@ async function setMembers(id: string, members: string[]): Promise<Pod | null> {
return updated ?? null; return updated ?? null;
} }
export async function addMember(id: string, rawName: string): Promise<Pod | null> { export async function addMember(id: string, rawName: unknown): Promise<Pod | null> {
if (typeof rawName !== 'string') throw new Error('member name must be a string');
const name = rawName.trim(); const name = rawName.trim();
if (!name) throw new Error('member name is required'); 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); const pod = await getPod(id);
if (!pod) return null; if (!pod) return null;
if (pod.members.some((m) => m.toLowerCase() === name.toLowerCase())) return pod; 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]); return setMembers(id, [...pod.members, name]);
} }
export async function removeMember(id: string, rawName: string): Promise<Pod | null> { export async function removeMember(id: string, rawName: string): Promise<Pod | null> {
const name = rawName.trim().toLowerCase(); const name = String(rawName).trim().toLowerCase();
const pod = await getPod(id); const pod = await getPod(id);
if (!pod) return null; if (!pod) return null;
return setMembers( const filtered = pod.members.filter((m) => m.toLowerCase() !== name);
id, if (filtered.length === pod.members.length) return pod; // no-op: don't write / bump updatedAt
pod.members.filter((m) => m.toLowerCase() !== name), 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<void> { export async function seedDefaultPods(): Promise<void> {
const c = await collections(); const c = await collections();
if ((await c.pods.estimatedDocumentCount()) > 0) return; if ((await c.pods.estimatedDocumentCount()) > 0) return;
@@ -156,6 +196,8 @@ export async function seedDefaultPods(): Promise<void> {
updatedAt: ts, 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'); console.log('[pods] seeded default pods');
} }
+8
View File
@@ -51,8 +51,16 @@ app.post('/api/sync-pr', async (req, res) => {
// Outcome ACK -> closes the continual-learning policy loop. // Outcome ACK -> closes the continual-learning policy loop.
app.post('/api/outcome', async (req, res) => { app.post('/api/outcome', async (req, res) => {
try {
const o = (req.body ?? {}) as Partial<InterventionOutcome>;
if (typeof o.interventionId !== 'string' || !o.interventionId) {
return res.status(400).json({ error: 'interventionId is required' });
}
await recordOutcome(req.body as InterventionOutcome); await recordOutcome(req.body as InterventionOutcome);
res.json({ ok: true }); 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. // Memory counts — quick way to confirm Mongo persistence is working.
+40 -20
View File
@@ -10,15 +10,17 @@ import { PodView } from './components/PodView.js';
export default function App() { export default function App() {
const [pods, setPods] = useState<Pod[]>([]); const [pods, setPods] = useState<Pod[]>([]);
const [loading, setLoading] = useState(true); const [loading, setLoading] = useState(true);
const [busy, setBusy] = useState(false); const [pending, setPending] = useState<Set<string>>(new Set());
const [error, setError] = useState<string | null>(null); const [error, setError] = useState<string | null>(null);
// join state // join state — keep the id and derive the pod so it never goes stale
const [joinedPod, setJoinedPod] = useState<Pod | null>(null); const [joinedPodId, setJoinedPodId] = useState<string | null>(null);
const [member, setMember] = useState(''); const [member, setMember] = useState('');
const [devMode, setDevMode] = useState(false); const [devMode, setDevMode] = useState(false);
const [room, setRoom] = useState<Room | null>(null); const [room, setRoom] = useState<Room | null>(null);
const joinedPod = joinedPodId ? (pods.find((p) => p.id === joinedPodId) ?? null) : null;
async function refresh() { async function refresh() {
setLoading(true); setLoading(true);
try { try {
@@ -35,40 +37,58 @@ export default function App() {
void refresh(); void refresh();
}, []); }, []);
/** Run a mutation, reflect the result in local state, surface errors. */ const startPending = (key: string) => setPending((s) => new Set(s).add(key));
async function mutate(fn: () => Promise<void>) { const endPending = (key: string) =>
setBusy(true); 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<void>) {
startPending(key);
setError(null); setError(null);
try { try {
await fn(); await fn();
} catch (e) { } catch (e) {
setError((e as Error).message); setError((e as Error).message);
} finally { } finally {
setBusy(false); endPending(key);
} }
} }
const upsert = (p: Pod) => setPods((cur) => cur.map((x) => (x.id === p.id ? p : x))); const upsert = (p: Pod) => setPods((cur) => cur.map((x) => (x.id === p.id ? p : x)));
const handleCreate = (input: PodInput) => // create rethrows so CreatePodForm can keep the user's input on failure
mutate(async () => { async function handleCreate(input: PodInput): Promise<void> {
startPending('new');
setError(null);
try {
const created = await api.createPod(input); const created = await api.createPod(input);
setPods((cur) => [...cur, created]); setPods((cur) => [...cur, created]);
}); } catch (e) {
setError((e as Error).message);
throw e;
} finally {
endPending('new');
}
}
const handleUpdate = (id: string, patch: PodInput) => 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) => const handleDelete = (id: string) =>
mutate(async () => { run(id, async () => {
await api.deletePod(id); await api.deletePod(id);
setPods((cur) => cur.filter((x) => x.id !== id)); setPods((cur) => cur.filter((x) => x.id !== id));
}); });
const handleAddMember = (id: string, name: string) => 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) => 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) { async function handleJoin(pod: Pod, who: string) {
setBusy(true); startPending(pod.id);
setError(null); setError(null);
try { try {
const identity = `${who}-${Math.random().toString(36).slice(2, 7)}`; const identity = `${who}-${Math.random().toString(36).slice(2, 7)}`;
@@ -76,18 +96,18 @@ export default function App() {
setRoom(result.room); setRoom(result.room);
setDevMode(result.mode === 'dev'); setDevMode(result.mode === 'dev');
setMember(who); setMember(who);
setJoinedPod(pod); setJoinedPodId(pod.id);
} catch (e) { } catch (e) {
setError((e as Error).message); setError((e as Error).message);
} finally { } finally {
setBusy(false); endPending(pod.id);
} }
} }
function handleLeave() { function handleLeave() {
room?.disconnect(); room?.disconnect();
setRoom(null); setRoom(null);
setJoinedPod(null); setJoinedPodId(null);
} }
return ( return (
@@ -133,7 +153,7 @@ export default function App() {
<PodCard <PodCard
key={pod.id} key={pod.id}
pod={pod} pod={pod}
busy={busy} busy={pending.has(pod.id)}
onJoin={handleJoin} onJoin={handleJoin}
onAddMember={handleAddMember} onAddMember={handleAddMember}
onRemoveMember={handleRemoveMember} onRemoveMember={handleRemoveMember}
@@ -141,7 +161,7 @@ export default function App() {
onDelete={handleDelete} onDelete={handleDelete}
/> />
))} ))}
<CreatePodForm busy={busy} onCreate={handleCreate} /> <CreatePodForm busy={pending.has('new')} onCreate={handleCreate} />
</div> </div>
)} )}
</main> </main>
+8 -3
View File
@@ -6,7 +6,7 @@ export function CreatePodForm({
onCreate, onCreate,
}: { }: {
busy: boolean; busy: boolean;
onCreate: (input: PodInput) => void; onCreate: (input: PodInput) => Promise<void>;
}) { }) {
const [open, setOpen] = useState(false); const [open, setOpen] = useState(false);
const [name, setName] = useState(''); const [name, setName] = useState('');
@@ -14,18 +14,23 @@ export function CreatePodForm({
const [description, setDescription] = useState(''); const [description, setDescription] = useState('');
const [firstMember, setFirstMember] = useState(''); const [firstMember, setFirstMember] = useState('');
function submit() { async function submit() {
if (!name.trim()) return; if (!name.trim()) return;
onCreate({ try {
await onCreate({
name: name.trim(), name: name.trim(),
repo: repo.trim(), repo: repo.trim(),
description: description.trim(), description: description.trim(),
members: firstMember.trim() ? [firstMember.trim()] : [], members: firstMember.trim() ? [firstMember.trim()] : [],
}); });
// only clear + close on success; on error keep the user's input
setName(''); setName('');
setDescription(''); setDescription('');
setFirstMember(''); setFirstMember('');
setOpen(false); setOpen(false);
} catch {
/* error surfaced by parent; keep inputs so nothing is lost */
}
} }
if (!open) { if (!open) {