From 52767bbba48b1cd043a39123dfba0de34e949404 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Sun, 19 Jul 2026 23:53:03 -0700 Subject: [PATCH 1/2] feat(mirrors): reject prefixes that overlap another product On a shared data connection (= same bucket), storage keys are built by literal concatenation, so if one product's slash-normalized prefix is a string-prefix of another's their keyspaces overlap. Add findPrefixConflict (reusing productsTable.listProductsByConnectionId) and enforce it in both updateMirrorPrefix (free-form input) and addProductMirror (guards a prefix_template missing {{repository_id}}). Overlap is nested-either- direction; root prefix overlaps everything. Replaces the earlier deferral. Co-Authored-By: Claude Opus 4.8 --- src/lib/actions/product-mirrors.test.ts | 113 ++++++++++++++++++++++++ src/lib/actions/product-mirrors.ts | 70 ++++++++++++++- 2 files changed, 181 insertions(+), 2 deletions(-) diff --git a/src/lib/actions/product-mirrors.test.ts b/src/lib/actions/product-mirrors.test.ts index 44b12a50..48b3297c 100644 --- a/src/lib/actions/product-mirrors.test.ts +++ b/src/lib/actions/product-mirrors.test.ts @@ -14,6 +14,7 @@ jest.mock("../clients", () => ({ productsTable: { fetchById: jest.fn(), update: jest.fn(), + listProductsByConnectionId: jest.fn(), }, dataConnectionsTable: { fetchById: jest.fn(), @@ -104,9 +105,26 @@ beforeEach(() => { mockIsAuthorized.mockReturnValue(true); mockCanManageDataConnection.mockResolvedValue(true); mockProductsTable.update.mockImplementation(async (p) => p); + mockProductsTable.listProductsByConnectionId.mockResolvedValue([]); mockDataConnectionsTable.fetchById.mockResolvedValue(s3Connection); }); +// A different product mirroring to `connectionId` at `prefix`, for overlap tests. +function siblingProduct( + productId: string, + connectionId: string, + prefix: string +): Product { + return { + account_id: "acct", + product_id: productId, + metadata: { + mirrors: { [connectionId]: mirror({ connection_id: connectionId, prefix }) }, + primary_mirror: connectionId, + }, + } as Product; +} + describe("addProductMirror", () => { test("rejects non-admins before any write", async () => { mockIsAdmin.mockReturnValue(false); @@ -147,6 +165,27 @@ describe("addProductMirror", () => { }); }); + test("rejects when the resolved prefix overlaps another product on the connection", async () => { + mockProductsTable.fetchById.mockResolvedValue(productWith({}, "")); + // s3Connection resolves to "acct/prod/"; a sibling nested under it overlaps. + mockProductsTable.listProductsByConnectionId.mockResolvedValue([ + siblingProduct("other-prod", "conn-a", "acct/prod/sub/"), + ]); + + const result = await addProductMirror( + FORM_STATE, + formDataFor({ + account_id: "acct", + product_id: "prod", + connection_id: "conn-a", + }) + ); + + expect(result.success).toBe(false); + expect(result.message).toMatch(/overlap/i); + expect(mockProductsTable.update).not.toHaveBeenCalled(); + }); + test("passes the read updated_at as an optimistic lock and reports conflicts", async () => { mockProductsTable.fetchById.mockResolvedValue({ ...productWith({}, ""), @@ -510,4 +549,78 @@ describe("updateMirrorPrefix", () => { expect(mockProductsTable.update).not.toHaveBeenCalled(); } ); + + test.each([ + ["nested under a sibling", "alice/foo/bar", "alice/foo/"], + ["containing a sibling", "alice/foo", "alice/foo/bar/"], + ])( + "rejects a prefix %s on the same connection", + async (_desc, prefix, siblingPrefix) => { + mockProductsTable.fetchById.mockResolvedValue( + productWith({ "conn-a": mirror({ connection_id: "conn-a" }) }, "conn-a") + ); + mockProductsTable.listProductsByConnectionId.mockResolvedValue([ + siblingProduct("other-prod", "conn-a", siblingPrefix), + ]); + + const result = await updateMirrorPrefix( + FORM_STATE, + formDataFor({ + account_id: "acct", + product_id: "prod", + mirror_key: "conn-a", + prefix, + }) + ); + + expect(result.success).toBe(false); + expect(result.message).toMatch(/overlaps/i); + expect(mockProductsTable.update).not.toHaveBeenCalled(); + } + ); + + test("ignores the product being edited when checking overlap", async () => { + mockProductsTable.fetchById.mockResolvedValue( + productWith({ "conn-a": mirror({ connection_id: "conn-a" }) }, "conn-a") + ); + // The scan returns this very product; its own prefix must not block itself. + mockProductsTable.listProductsByConnectionId.mockResolvedValue([ + siblingProduct("prod", "conn-a", "alice/foo/"), + ]); + + const result = await updateMirrorPrefix( + FORM_STATE, + formDataFor({ + account_id: "acct", + product_id: "prod", + mirror_key: "conn-a", + prefix: "alice/foo/bar/", + }) + ); + + expect(result.success).toBe(true); + expect(mockProductsTable.update).toHaveBeenCalled(); + }); + + test("allows a non-overlapping sibling prefix", async () => { + mockProductsTable.fetchById.mockResolvedValue( + productWith({ "conn-a": mirror({ connection_id: "conn-a" }) }, "conn-a") + ); + mockProductsTable.listProductsByConnectionId.mockResolvedValue([ + siblingProduct("other-prod", "conn-a", "bob/other/"), + ]); + + const result = await updateMirrorPrefix( + FORM_STATE, + formDataFor({ + account_id: "acct", + product_id: "prod", + mirror_key: "conn-a", + prefix: "alice/foo/", + }) + ); + + expect(result.success).toBe(true); + expect(mockProductsTable.update).toHaveBeenCalled(); + }); }); diff --git a/src/lib/actions/product-mirrors.ts b/src/lib/actions/product-mirrors.ts index a5b2f7ed..85290a43 100644 --- a/src/lib/actions/product-mirrors.ts +++ b/src/lib/actions/product-mirrors.ts @@ -7,6 +7,7 @@ import { productsTable, dataConnectionsTable } from "../clients"; import { Actions, DataProvider, + Product, ProductMirror, resolveMirrorPrefix, } from "@/types"; @@ -29,6 +30,44 @@ function isConcurrentEdit(error: unknown): boolean { ); } +// Slash-terminate so prefixes compare on directory boundaries; root ("") stays +// "". Keys are built by literal concatenation (`${prefix}${key}`), so without a +// trailing slash "acct/prod" would also match "acct/prod2/". +const normalizePrefix = (p: string) => (p === "" || p.endsWith("/") ? p : `${p}/`); + +// Prefixes on the same connection (= same bucket) overlap when one is nested in +// the other (either direction). `"".startsWith(x)` semantics make a root prefix +// overlap everything, which is correct — a root prefix owns the whole bucket. +const prefixesOverlap = (a: string, b: string) => + a.startsWith(b) || b.startsWith(a); + +// The first *other* product mirroring to this connection whose prefix overlaps +// `prefix`, or null. Excludes the product being edited (self). +// +// ponytail: listProductsByConnectionId is a full table scan (no GSI on the +// mirror map's connection_id). Fine for this low-traffic admin path; if product +// volume explodes, add a denormalized connection index. +async function findPrefixConflict( + connectionId: string, + prefix: string, + self: { accountId: string; productId: string } +): Promise { + const normalized = normalizePrefix(prefix); + const siblings = await productsTable.listProductsByConnectionId(connectionId); + for (const p of siblings) { + if (p.account_id === self.accountId && p.product_id === self.productId) { + continue; + } + const m = Object.values(p.metadata.mirrors).find( + (mm) => mm.connection_id === connectionId + ); + if (m && prefixesOverlap(normalized, normalizePrefix(m.prefix))) { + return p; + } + } + return null; +} + export async function addProductMirror( _prevState: FormState, formData: FormData @@ -102,6 +141,22 @@ export async function addProductMirror( productId ); + // Guards against a prefix_template that omits {{repository.repository_id}} + // (two products resolve to the same prefix) or attaching a second product at + // a shared root prefix. + const conflict = await findPrefixConflict(connectionId, prefix, { + accountId, + productId, + }); + if (conflict) { + return { + fieldErrors: {}, + data: formData, + message: `This connection's prefix would overlap product ${conflict.account_id}/${conflict.product_id}. Adjust the connection's prefix template.`, + success: false, + }; + } + const isFirst = Object.keys(product.metadata.mirrors).length === 0; const storageTypeByProvider: Record< @@ -328,8 +383,6 @@ export async function updateMirrorPrefix( // concatenation (`${prefix}${key}`), so reject traversal/leading-slash and // force a trailing separator — without it "acct/prod" also matches keys under // "acct/prod2/" on a shared connection. - // ponytail: no cross-product collision scan; managing the connection already - // grants broad access. Add a scan if prefix overlap becomes a real problem. if (rawPrefix.includes("..") || rawPrefix.startsWith("/")) { return { fieldErrors: {}, @@ -384,6 +437,19 @@ export async function updateMirrorPrefix( }; } + const conflict = await findPrefixConflict(existing.connection_id, prefix, { + accountId, + productId, + }); + if (conflict) { + return { + fieldErrors: {}, + data: formData, + message: `This prefix overlaps product ${conflict.account_id}/${conflict.product_id} on the same data connection. Choose a non-overlapping prefix.`, + success: false, + }; + } + const updatedProduct = { ...product, metadata: { From 87f934b00234353cc302734eee4e998f2427f84e Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 20 Jul 2026 12:11:27 -0700 Subject: [PATCH 2/2] fix(mirrors): store the slash-normalized prefix in addProductMirror MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The overlap check normalized both sides for comparison, but the raw resolveMirrorPrefix() value was still stored and later concatenated into keys. A prefix_template without a trailing slash (optional in the schema) therefore stored "acct/prod", which literally matches keys under a sibling "acct/prod2/" — the exact overlap the check exists to prevent, slipping past it silently. Normalize once, before both checking and storing. Co-Authored-By: Claude Opus 4.8 --- src/lib/actions/product-mirrors.test.ts | 23 +++++++++++++++++++++++ src/lib/actions/product-mirrors.ts | 10 ++++++---- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/lib/actions/product-mirrors.test.ts b/src/lib/actions/product-mirrors.test.ts index 48b3297c..be05c5f4 100644 --- a/src/lib/actions/product-mirrors.test.ts +++ b/src/lib/actions/product-mirrors.test.ts @@ -231,6 +231,29 @@ describe("addProductMirror", () => { expect(updated.metadata.mirrors["conn-a"].prefix).toBe(""); }); + test("slash-terminates a prefix from a template missing its trailing slash", async () => { + // Without this, the stored "acct/prod" literally matches keys under a + // sibling "acct/prod2/", overlapping despite the normalized overlap check. + mockProductsTable.fetchById.mockResolvedValue(productWith({}, "")); + mockDataConnectionsTable.fetchById.mockResolvedValue({ + data_connection_id: "conn-a", + prefix_template: "{{repository.account_id}}/{{repository.repository_id}}", + details: { provider: "s3" }, + } as DataConnection); + + await addProductMirror( + FORM_STATE, + formDataFor({ + account_id: "acct", + product_id: "prod", + connection_id: "conn-a", + }) + ); + + const updated = mockProductsTable.update.mock.calls[0][0]; + expect(updated.metadata.mirrors["conn-a"].prefix).toBe("acct/prod/"); + }); + test("maps a GCP connection to the gcs storage type", async () => { mockProductsTable.fetchById.mockResolvedValue(productWith({}, "")); mockDataConnectionsTable.fetchById.mockResolvedValue({ diff --git a/src/lib/actions/product-mirrors.ts b/src/lib/actions/product-mirrors.ts index 85290a43..b519002a 100644 --- a/src/lib/actions/product-mirrors.ts +++ b/src/lib/actions/product-mirrors.ts @@ -135,10 +135,12 @@ export async function addProductMirror( }; } - const prefix = resolveMirrorPrefix( - connection.prefix_template, - accountId, - productId + // Normalize before storing (not just comparing): a prefix_template may omit + // the trailing slash (it's optional in the schema), and the raw value is + // what's later concatenated into keys — so "acct/prod" would match keys + // under "acct/prod2/" and slip past the overlap check below. + const prefix = normalizePrefix( + resolveMirrorPrefix(connection.prefix_template, accountId, productId) ); // Guards against a prefix_template that omits {{repository.repository_id}}