diff --git a/apps/web/app/(authed)/memories/[id]/page.tsx b/apps/web/app/(authed)/memories/[id]/page.tsx index 272ccc9..78e169c 100644 --- a/apps/web/app/(authed)/memories/[id]/page.tsx +++ b/apps/web/app/(authed)/memories/[id]/page.tsx @@ -236,6 +236,7 @@ export default async function MemoryDetailPage({ {!isEditing && canWrite ? (
+ diff --git a/apps/web/app/(authed)/snippets/[name]/page.tsx b/apps/web/app/(authed)/snippets/[name]/page.tsx index 988170d..53bf0f8 100644 --- a/apps/web/app/(authed)/snippets/[name]/page.tsx +++ b/apps/web/app/(authed)/snippets/[name]/page.tsx @@ -287,6 +287,7 @@ export default async function SnippetDetailPage({ + {snippet.scope === "project" && snippet.projectKey ? ( ) : null} diff --git a/apps/web/lib/access.ts b/apps/web/lib/access.ts index 24136db..2282c38 100644 --- a/apps/web/lib/access.ts +++ b/apps/web/lib/access.ts @@ -79,13 +79,12 @@ export async function getAccessibleProjects( .innerJoin(projects, eq(projects.id, projectShares.projectId)) .where(inArray(groups.name, groupNames)); + // If two of the user's groups both share the same project at different + // levels, keep the strongest: owner > rw > ro. The DB may emit the same + // project twice (once per group), so we collapse by taking the max. for (const r of shared) { - const existing = ownedMap.get(r.projectId); - if (existing) continue; // owner already wins - // If two of the user's groups both share the same project at - // different levels, keep the stronger one (rw beats ro). const prior = ownedMap.get(r.projectId); - if (prior && prior.access === "rw") continue; + if (prior?.access === "owner" || prior?.access === "rw") continue; ownedMap.set(r.projectId, { projectId: r.projectId, access: r.access as "ro" | "rw", diff --git a/apps/web/lib/mcp/tools.ts b/apps/web/lib/mcp/tools.ts index f7f0881..618b88c 100644 --- a/apps/web/lib/mcp/tools.ts +++ b/apps/web/lib/mcp/tools.ts @@ -9,6 +9,7 @@ import { } from "@/lib/db/schema"; import { MemoryIdInput, + MemoryDeleteInput, MemoryListInput, MemorySearchInput, MemoryUpdateInput, @@ -496,25 +497,33 @@ const memoryGet: ToolDef = { const memoryDelete: ToolDef = { name: "memory.delete", description: - "Soft-delete a memory when it becomes stale or wrong — e.g., the user changes a preference, or a fact you saved turns out to be incorrect. ALWAYS prefer memory.update over delete-then-write for content corrections; only delete when the memory genuinely shouldn't exist anymore. Soft delete preserves the audit trail.", + "Soft-delete a memory when it becomes stale or wrong — e.g., the user changes a preference, or a fact you saved turns out to be incorrect. ALWAYS prefer memory.update over delete-then-write for content corrections; only delete when the memory genuinely shouldn't exist anymore. Soft delete preserves the audit trail. Pass `version` (returned by memory.get / memory.list) to detect concurrent edits — in shared projects another member may have updated the row since you read it.", inputSchema: { type: "object", - properties: { id: { type: "string", format: "uuid" } }, + properties: { + id: { type: "string", format: "uuid" }, + version: { + type: "integer", + minimum: 0, + description: + "If supplied, the delete fails with a concurrent-edit error when the row's current version doesn't match. Recommended for shared projects.", + }, + }, required: ["id"], }, async handler(args, ctx) { - const parsed = MemoryIdInput.safeParse(args); + const parsed = MemoryDeleteInput.safeParse(args); if (!parsed.success) return err(parsed.error.message); - // Look up the row first to authorize. We can't rely on a - // single-statement WHERE clause because shared-project writes - // need a per-project access check. + // Look up the row first to authorize and capture its current version + // for the CAS. Shared-project writes need a per-project access check. const target = await db .select({ id: memories.id, userId: memories.userId, projectId: memories.projectId, scope: memories.scope, + version: memories.version, }) .from(memories) .where(and(eq(memories.id, parsed.data.id), isNull(memories.deletedAt))) @@ -530,13 +539,23 @@ const memoryDelete: ToolDef = { if (!allowed) return err("no write access to this project"); } + // Optimistic-lock CAS: pin to the caller-supplied version when given, + // else the version we just read in this handler. The 0-row response + // tells us a peer raced us. + const expectedVersion = parsed.data.version ?? m.version; const updated = await db .update(memories) .set({ deletedAt: new Date(), lastEditedBy: ctx.userId }) - .where(and(eq(memories.id, parsed.data.id), isNull(memories.deletedAt))) + .where( + and( + eq(memories.id, parsed.data.id), + eq(memories.version, expectedVersion), + isNull(memories.deletedAt), + ), + ) .returning({ id: memories.id }); - if (!updated[0]) return err("not found"); + if (!updated[0]) return err(CONCURRENT_EDIT_ERROR); await db.insert(auditLog).values({ userId: ctx.userId, @@ -743,6 +762,9 @@ const memorySearch: ToolDef = { } const topIds = result.hits.map((h) => h.id); + // Re-filter on deletedAt — close a tiny TOCTOU window where a row + // could be soft-deleted between the visibility-aware search and this + // re-fetch. Visibility itself is already enforced by `searchMemories`. const rows = await db .select({ id: memories.id, @@ -756,7 +778,7 @@ const memorySearch: ToolDef = { updatedAt: memories.updatedAt, }) .from(memories) - .where(inArray(memories.id, topIds)); + .where(and(inArray(memories.id, topIds), isNull(memories.deletedAt))); const byId = new Map(rows.map((r) => [r.id, r])); const items = result.hits.flatMap((hit) => { @@ -836,16 +858,11 @@ const snippetPut: ToolDef = { return err("scope=project requires `project` (or X-Project-Key header)"); } - if (parsed.data.scope === "project") { - const exists = await resolveProjectId(ctx, projectKey!); - // It's OK for the project not to exist — putSnippet will create - // it owned by the caller. But if it DOES exist as a shared - // project, we need rw to write through it; the helper enforces - // that. - void exists; - } - try { + // Authorization for shared-project writes lives inside putSnippet: + // if the project exists and the caller lacks rw access on it, the + // helper throws. If the project doesn't exist, the helper creates + // it owned by the caller — auto-upsert semantics. const { snippet, inserted } = await putSnippet(ctx.userId, { name: parsed.data.name, body: parsed.data.body, @@ -996,13 +1013,19 @@ const snippetList: ToolDef = { const snippetDelete: ToolDef = { name: "snippet.delete", description: - "Soft-delete a snippet by name when it becomes stale or wrong — e.g., the user revamps a template and the old version shouldn't be reachable anymore. ALWAYS prefer snippet.put with the same name (which replaces in place) over delete-then-put when you're just refining the body. Only delete when the snippet genuinely shouldn't exist. Provide `scope` (and `project` for project-scope) to disambiguate when the same name exists in multiple scopes.", + "Soft-delete a snippet by name when it becomes stale or wrong — e.g., the user revamps a template and the old version shouldn't be reachable anymore. ALWAYS prefer snippet.put with the same name (which replaces in place) over delete-then-put when you're just refining the body. Only delete when the snippet genuinely shouldn't exist. Provide `scope` (and `project` for project-scope) to disambiguate when the same name exists in multiple scopes. Pass `version` (returned by snippet.get / snippet.list) to detect concurrent edits on shared-project snippets.", inputSchema: { type: "object", properties: { name: { type: "string", description: "Exact snippet name." }, scope: { type: "string", enum: ["project", "user"] }, project: { type: "string", description: "Project key (required for scope='project')." }, + version: { + type: "integer", + minimum: 0, + description: + "If supplied, the delete fails with a concurrent-edit error when the row's current version doesn't match. Recommended for shared projects.", + }, }, required: ["name"], }, @@ -1017,6 +1040,7 @@ const snippetDelete: ToolDef = { scope: parsed.data.scope, projectKey: requestedKey, groupNames: ctx.groups, + version: parsed.data.version, }); if (!deleted) return err(`snippet '${parsed.data.name}' not found`); diff --git a/apps/web/lib/memory-actions.ts b/apps/web/lib/memory-actions.ts index 8264a89..4daae84 100644 --- a/apps/web/lib/memory-actions.ts +++ b/apps/web/lib/memory-actions.ts @@ -2,7 +2,7 @@ import { revalidatePath } from "next/cache"; import { redirect } from "next/navigation"; -import { and, eq, isNull } from "drizzle-orm"; +import { and, eq, inArray, isNull } from "drizzle-orm"; import { auth } from "@/auth"; import { db } from "@/lib/db/client"; import { memories, projects, auditLog } from "@/lib/db/schema"; @@ -11,12 +11,13 @@ import { resolveProjectId, upsertProject } from "@/lib/projects"; import { MemoryWriteInput, MemoryUpdateInput, - MemoryIdInput, + MemoryDeleteInput, } from "@shared-memory/schemas"; import { CONCURRENT_EDIT_ERROR, canWriteProject, getUserGroupNames, + readableProjectIds, } from "@/lib/access"; /** @@ -73,11 +74,24 @@ export async function createMemoryAction(formData: FormData) { if (owned) { projectId = owned; } else { - const sharedRow = await db - .select({ id: projects.id }) - .from(projects) - .where(eq(projects.key, parsed.data.project)) - .limit(1); + // Restrict the by-key lookup to projects the user can actually + // read. Without this, a different user's project with the same + // key string could be selected (`projects.key` is unique per user, + // not globally), opening a cross-user write hazard. + const readableIds = await readableProjectIds(userId, groupNames); + const sharedRow = + readableIds.length > 0 + ? await db + .select({ id: projects.id }) + .from(projects) + .where( + and( + eq(projects.key, parsed.data.project), + inArray(projects.id, readableIds), + ), + ) + .limit(1) + : []; if (sharedRow[0]) { const allowed = await canWriteProject(userId, groupNames, sharedRow[0].id); if (!allowed) { @@ -218,12 +232,20 @@ export async function updateMemoryAction(formData: FormData) { if (existingId) { projectId = existingId; } else { - // Try a shared project with this key. - const sharedRow = await db - .select({ id: projects.id }) - .from(projects) - .where(eq(projects.key, projectKey)) - .limit(1); + // Restrict the shared-project lookup to projects the user can + // actually read (`projects.key` is unique per user, not globally, + // so an unscoped key match could resolve another user's project). + const readableIds = await readableProjectIds(userId, groupNames); + const sharedRow = + readableIds.length > 0 + ? await db + .select({ id: projects.id }) + .from(projects) + .where( + and(eq(projects.key, projectKey), inArray(projects.id, readableIds)), + ) + .limit(1) + : []; if (sharedRow[0]) { const allowed = await canWriteProject(userId, groupNames, sharedRow[0].id); if (!allowed) { @@ -299,7 +321,15 @@ export async function deleteMemoryAction(formData: FormData) { const userId = await requireUserId(); const groupNames = await getUserGroupNames(userId); const id = String(formData.get("id") ?? ""); - const parsed = MemoryIdInput.safeParse({ id }); + const rawVersion = formData.get("version"); + const version = + typeof rawVersion === "string" && rawVersion.length > 0 + ? Number.parseInt(rawVersion, 10) + : undefined; + const parsed = MemoryDeleteInput.safeParse({ + id, + version: Number.isFinite(version) ? version : undefined, + }); if (!parsed.success) throw new Error(parsed.error.issues[0]!.message); // Authorize delete: same rule as update — owner OR rw on the project. @@ -309,6 +339,7 @@ export async function deleteMemoryAction(formData: FormData) { scope: memories.scope, projectId: memories.projectId, userId: memories.userId, + version: memories.version, }) .from(memories) .where(and(eq(memories.id, parsed.data.id), isNull(memories.deletedAt))) @@ -323,13 +354,23 @@ export async function deleteMemoryAction(formData: FormData) { if (!allowed) throw new Error("you don't have write access to this project"); } + // CAS on version so a peer's concurrent edit can't be silently overwritten + // by this delete. Form may or may not supply version; fall back to the row + // we just read to keep behaviour deterministic. + const expectedVersion = parsed.data.version ?? row.version; const updated = await db .update(memories) .set({ deletedAt: new Date(), lastEditedBy: userId }) - .where(and(eq(memories.id, parsed.data.id), isNull(memories.deletedAt))) + .where( + and( + eq(memories.id, parsed.data.id), + eq(memories.version, expectedVersion), + isNull(memories.deletedAt), + ), + ) .returning({ id: memories.id }); - if (!updated[0]) throw new Error("not found"); + if (!updated[0]) throw new Error(CONCURRENT_EDIT_ERROR); await db.insert(auditLog).values({ userId, diff --git a/apps/web/lib/snippet-actions.ts b/apps/web/lib/snippet-actions.ts index d8a053f..719413e 100644 --- a/apps/web/lib/snippet-actions.ts +++ b/apps/web/lib/snippet-actions.ts @@ -155,10 +155,16 @@ export async function deleteSnippetAction(formData: FormData) { const scope = formData.get("scope") as "project" | "user" | null; const projectRaw = (formData.get("project") as string | null)?.trim(); + const rawVersion = formData.get("version"); + const version = + typeof rawVersion === "string" && rawVersion.length > 0 + ? Number.parseInt(rawVersion, 10) + : undefined; const payload = { name: String(formData.get("name") ?? "").trim(), scope: scope ?? undefined, project: scope === "project" ? projectRaw || undefined : undefined, + version: Number.isFinite(version) ? version : undefined, }; const parsed = SnippetDeleteInput.safeParse(payload); @@ -171,6 +177,7 @@ export async function deleteSnippetAction(formData: FormData) { scope: parsed.data.scope, projectKey: parsed.data.project, groupNames, + version: parsed.data.version, }); if (!deleted) throw new Error("not found"); diff --git a/apps/web/lib/snippets.ts b/apps/web/lib/snippets.ts index f05303c..668aab8 100644 --- a/apps/web/lib/snippets.ts +++ b/apps/web/lib/snippets.ts @@ -220,11 +220,20 @@ export async function putSnippet( if (owned) { projectId = owned; } else { - const sharedRow = await db - .select({ id: projects.id }) - .from(projects) - .where(eq(projects.key, projectKey)) - .limit(1); + // Restrict by-key lookup to projects the user can actually read — + // `projects.key` is unique per user, not globally, so an unscoped + // match could resolve another user's project entirely. + const readableIds = await readableProjectIds(userId, groupNames); + const sharedRow = + readableIds.length > 0 + ? await db + .select({ id: projects.id }) + .from(projects) + .where( + and(eq(projects.key, projectKey), inArray(projects.id, readableIds)), + ) + .limit(1) + : []; if (sharedRow[0]) { const allowed = await canWriteProject(userId, groupNames, sharedRow[0].id); if (!allowed) { @@ -386,9 +395,10 @@ export async function softDeleteSnippet( scope?: "project" | "user"; projectKey?: string; groupNames?: string[]; + version?: number; }, ): Promise<{ id: string; scope: "project" | "user"; projectKey: string | null } | null> { - const { groupNames = [] } = args; + const { groupNames = [], version } = args; const target = await getSnippet(userId, args); if (!target) return null; @@ -403,10 +413,19 @@ export async function softDeleteSnippet( if (!allowed) throw new Error("you don't have write access to this project"); } - await db + // CAS on version so a peer's concurrent edit can't be silently dropped + // by this delete. Caller-supplied version wins; else we use the version + // we just read in `getSnippet` for in-handler consistency. + const expectedVersion = version ?? target.version; + const updated = await db .update(snippets) .set({ deletedAt: new Date(), lastEditedBy: userId }) - .where(eq(snippets.id, target.id)); + .where(and(eq(snippets.id, target.id), eq(snippets.version, expectedVersion))) + .returning({ id: snippets.id }); + + if (!updated[0]) { + throw new Error(CONCURRENT_EDIT_ERROR_SNIPPET); + } return { id: target.id, diff --git a/packages/schemas/src/index.ts b/packages/schemas/src/index.ts index d7fe1f8..f1d4392 100644 --- a/packages/schemas/src/index.ts +++ b/packages/schemas/src/index.ts @@ -47,6 +47,15 @@ export const MemoryIdInput = z.object({ }); export type MemoryIdInput = z.infer; +// memory.delete may CAS on `version` to avoid clobbering a concurrent edit +// (shared projects allow co-edit, so the version a caller observed at +// load time can race a peer's update). +export const MemoryDeleteInput = z.object({ + id: z.string().uuid(), + version: z.number().int().nonnegative().optional(), +}); +export type MemoryDeleteInput = z.infer; + export const MemoryUpdateInput = z.object({ id: z.string().uuid(), content: MemoryContent.optional(), @@ -161,6 +170,8 @@ export const SnippetDeleteInput = z name: SnippetName, scope: MemoryScope.optional(), project: ProjectKey.optional(), + // Optional CAS for co-edit safety on shared snippets. + version: z.number().int().nonnegative().optional(), }) .refine(scopeProjectRefinement.check, { message: scopeProjectRefinement.message }); export type SnippetDeleteInput = z.infer;