cybercove-labs/shared-memory
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address Phase 4 code review (5 findings + symmetric snippet fix)

Browse Source 
1. lib/snippets.ts + lib/memory-actions.ts (×2): shared-project key
   lookups were querying `projects` by key with no visibility scope. Since
   `projects.key` is unique per user (not global), the unscoped match
   could resolve another user's project entirely. Restricted the lookups
   to `readableProjectIds(userId, groupNames)` — own + shared only.

2. lib/access.ts getAccessibleProjects: a stray `if (existing) continue`
   inside the share-collapse loop short-circuited on the first match,
   killing the rw-beats-ro upgrade path. Two-group cases where one share
   was ro and another rw on the same project were incorrectly resolved
   as ro. Replaced with explicit owner/rw skip.

3. lib/mcp/tools.ts snippetPut: removed a dead `void exists` block that
   looked like an authorization pre-flight but was actually a no-op —
   real write authorization lives inside putSnippet, called next. Added
   a comment at the call site documenting where the check is.

4. memory.delete + snippet.delete: previously had no optimistic-lock
   CAS, so a concurrent peer edit could be silently overwritten by a
   delete on a stale view. Added optional `version` to MemoryDeleteInput
   (new) and SnippetDeleteInput (extended); UPDATE WHERE now CASes on
   version; 0-row response surfaces CONCURRENT_EDIT_ERROR. Web detail
   pages pass `version` through hidden form inputs. When the caller
   doesn't supply a version, falls back to the version we just read in
   the same handler for in-handler consistency.

5. lib/mcp/tools.ts memorySearch re-fetch: missing `isNull(deletedAt)`
   on the post-search row hydration left a TOCTOU window where a row
   soft-deleted between the search and the re-fetch would be returned.
   Visibility is still enforced by searchMemories itself.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
shadowdao
2026-05-17 10:06:10 -07:00
co-authored by Claude Opus 4.7
parent 3f17a5b2d6
commit a44b834a78
8 changed files with 151 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/web/app/(authed)/memories/[id]/page.tsx
+1
View File
@@ -236,6 +236,7 @@ export default async function MemoryDetailPage({
{!isEditing && canWrite ? (
<form action={deleteMemoryAction} className="flex justify-end">
<input type="hidden" name="id" value={m.id} />
<input type="hidden" name="version" value={m.version} />
<Button type="submit" variant="danger" size="sm">
Delete memory
</Button>
apps/web/app/(authed)/snippets/[name]/page.tsx
+1
View File
@@ -287,6 +287,7 @@ export default async function SnippetDetailPage({
<form action={deleteSnippetAction} className="flex justify-end">
<input type="hidden" name="name" value={snippet.name} />
<input type="hidden" name="scope" value={snippet.scope} />
<input type="hidden" name="version" value={snippet.version} />
{snippet.scope === "project" && snippet.projectKey ? (
<input type="hidden" name="project" value={snippet.projectKey} />
) : null}
apps/web/lib/access.ts
+4 -5
View File
@@ -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",
apps/web/lib/mcp/tools.ts
+43 -19
View File
@@ -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`);
apps/web/lib/memory-actions.ts
+57 -16
View File
@@ -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,
apps/web/lib/snippet-actions.ts
+7
View File
@@ -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");
apps/web/lib/snippets.ts
+27 -8
View File
@@ -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,