Skip to content

Commit 91c6d07

Browse files
committed
fix(account-deletion): bound and harden the storage reference guard
A stored URL with malformed percent-encoding (for example `/api/files/serve/%E0%A4%A`, which any workspace admin can store as a logo URL) made extractStorageKey throw. The guard then rejected on every call, so every storage purge on the platform failed closed and deleted nothing. The guard now parses each value under its own try and falls back to the raw, undecoded path. extractStorageKey itself is unchanged for its other callers. The URL-column check read six unindexed tables on almost every guard call. Only keys under the prefixes the file-serve route answers without a row or a session (profile-pictures/, og-images/, workspace-logos/, organization-logos/, per isPublicByKeyPrefix in the serve route) can be live through a stored URL alone. The guard now reads URL columns only for such keys, and only rows containing the key's longest URL-safe run, which percent-encoding never splits. Each run is read at most once per guard, 50 per query. A key with no run long enough falls back to the unfiltered read. Lookup failures still throw, so nothing is deleted. The workspace sweep now defers with a distinct reason when a full local page removes nothing, so the test for it can tell a stall from a deadline.
1 parent d4475f0 commit 91c6d07

2 files changed

Lines changed: 131 additions & 24 deletions

File tree

‎apps/sim/lib/users/storage-sweep.test.ts‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -191,11 +191,13 @@ describe('deleted workspace storage sweep', () => {
191191

192192
const result = await sweep(
193193
{ version: 1, workspaceId: 'ws1' },
194-
outboxContext({ deadlineAt: Date.now() + 5_500 })
194+
outboxContext({ deadlineAt: Date.now() + 120_000 })
195195
)
196196

197-
expect(result).toMatchObject({ outcome: 'deferred' })
198-
expect(result).not.toMatchObject({ consumeAttempt: false })
197+
expect(result).toEqual({
198+
outcome: 'deferred',
199+
reason: 'Workspace storage sweep removed nothing from a full listing page',
200+
})
199201
})
200202

201203
it.each([
@@ -286,13 +288,42 @@ describe('account storage object cleanup', () => {
286288
expect(await exists('copilot/abc/private.txt')).toBe(false)
287289
})
288290

289-
it('keeps a key a stored asset URL names', async () => {
291+
it('keeps a publicly served key a stored asset URL names', async () => {
292+
await put('workspace-logos/ws9/icon.png')
293+
queueTableRows(schemaMock.oauthClient, [
294+
{ value: '/api/files/serve/workspace-logos%2Fws9%2Ficon.png' },
295+
])
296+
297+
await cleanup(
298+
payload([{ key: 'workspace-logos/ws9/icon.png', context: 'workspace-logos' }]),
299+
outboxContext()
300+
)
301+
302+
expect(await exists('workspace-logos/ws9/icon.png')).toBe(true)
303+
})
304+
305+
it('reads no URL column for keys that are never served without a row', async () => {
290306
await put('copilot/abc/icon.png')
291307
queueTableRows(schemaMock.oauthClient, [{ value: '/api/files/serve/copilot%2Fabc%2Ficon.png' }])
292308

293309
await cleanup(payload([{ key: 'copilot/abc/icon.png', context: 'copilot' }]), outboxContext())
294310

295-
expect(await exists('copilot/abc/icon.png')).toBe(true)
311+
expect(await exists('copilot/abc/icon.png')).toBe(false)
312+
for (const table of ['organization', 'customBlock', 'chat', 'oauthClient'] as const)
313+
expect(dbChainMockFns.from).not.toHaveBeenCalledWith(schemaMock[table])
314+
})
315+
316+
it('still purges when a stored URL has malformed percent-encoding', async () => {
317+
await put('workspace-logos/ws9/old.png')
318+
queueTableRows(schemaMock.user, [])
319+
queueTableRows(schemaMock.user, [{ value: '/api/files/serve/%E0%A4%A' }])
320+
321+
await cleanup(
322+
payload([{ key: 'workspace-logos/ws9/old.png', context: 'workspace-logos' }]),
323+
outboxContext()
324+
)
325+
326+
expect(await exists('workspace-logos/ws9/old.png')).toBe(false)
296327
})
297328

298329
it.each([

‎apps/sim/lib/users/storage-sweep.ts‎

Lines changed: 95 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -224,10 +224,13 @@ function collectStrings(value: unknown, into: string[]): void {
224224
}
225225
}
226226

227+
const SERVE_PATH = '/api/files/serve/'
228+
227229
/**
228230
* The storage keys a stored URL can name: a `/api/files/serve/...` path (relative or absolute,
229231
* with or without a provider segment), or a direct bucket URL in virtual-host or path style.
230-
* Over-matching only keeps an object.
232+
* Over-matching only keeps an object. Never throws: a value whose percent-encoding is malformed
233+
* falls back to its raw path, so one bad stored string cannot disable every purge.
231234
*/
232235
function keysNamedByUrl(value: string): string[] {
233236
const keys = new Set<string>()
@@ -238,39 +241,85 @@ function keysNamedByUrl(value: string): string[] {
238241
keys.add(decodeURIComponent(candidate))
239242
} catch {}
240243
}
241-
const path = extractStorageKey(value.trim()).replace(/^\/+/, '')
244+
const trimmed = value.trim()
245+
let path: string
246+
try {
247+
path = extractStorageKey(trimmed)
248+
} catch {
249+
const raw = trimmed.split('?')[0]
250+
const serveAt = raw.indexOf(SERVE_PATH)
251+
path = serveAt >= 0 ? raw.slice(serveAt + SERVE_PATH.length) : raw
252+
}
253+
path = path.replace(/^\/+/, '')
242254
add(path)
243255
const slash = path.indexOf('/')
244256
if (slash > 0) add(path.slice(slash + 1))
245257
return [...keys]
246258
}
247259

260+
/**
261+
* Key prefixes the file-serve route answers without a row or a session
262+
* (`isPublicByKeyPrefix` in `app/api/files/serve/[...path]/route.ts`). Only these objects can be
263+
* live through a stored URL alone, so only they need the URL-column check.
264+
*/
265+
const PUBLIC_SERVE_PREFIXES = [
266+
'profile-pictures/',
267+
'og-images/',
268+
'workspace-logos/',
269+
'organization-logos/',
270+
] as const
271+
272+
const MIN_NEEDLE_LENGTH = 4
273+
const NEEDLES_PER_QUERY = 50
274+
275+
/**
276+
* The longest run of URL-safe characters after a key's root. Percent-encoding never splits such a
277+
* run, so any stored URL naming the key contains it; `null` when the key has none long enough.
278+
*/
279+
function urlNeedle(key: string): string | null {
280+
let best = ''
281+
for (const run of key.slice(key.indexOf('/') + 1).match(/[A-Za-z0-9_-]+/g) ?? [])
282+
if (run.length > best.length) best = run
283+
return best.length >= MIN_NEEDLE_LENGTH ? best : null
284+
}
285+
248286
/**
249287
* Keys named by the columns that hold a stored asset as a URL rather than a row:
250288
* `user.image`, `workspace.logo_url`, `organization.logo`, `organization.whitelabel_settings`,
251289
* `organization.metadata`, `custom_block.icon_url`, `chat.customizations` (deployed-chat logo
252-
* and avatar), and `oauth_client.icon`.
290+
* and avatar), and `oauth_client.icon`. With `needles`, reads only rows whose value contains one;
291+
* without, every row that holds a URL.
253292
*/
254-
async function loadUrlReferencedKeys(): Promise<Set<string>> {
293+
async function loadUrlReferencedKeys(needles: readonly string[] | null): Promise<Set<string>> {
294+
const matching = (column: SQLWrapper, fallback: SQL | undefined): SQL | undefined =>
295+
needles ? or(...needles.map((needle) => sql`${column}::text LIKE ${`%${needle}%`}`)) : fallback
255296
const values: string[] = []
256297
const sources = [
257298
db
258299
.select({ value: user.image })
259300
.from(user)
260-
.where(or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%'))),
301+
.where(
302+
matching(
303+
user.image,
304+
or(like(user.image, '%/api/files/serve/%'), like(user.image, '%profile-pictures/%'))
305+
)
306+
),
261307
db
262308
.select({ value: workspaceTable.logoUrl })
263309
.from(workspaceTable)
264-
.where(isNotNull(workspaceTable.logoUrl)),
310+
.where(matching(workspaceTable.logoUrl, isNotNull(workspaceTable.logoUrl))),
265311
db
266312
.select({ value: customBlock.iconUrl })
267313
.from(customBlock)
268-
.where(isNotNull(customBlock.iconUrl)),
314+
.where(matching(customBlock.iconUrl, isNotNull(customBlock.iconUrl))),
269315
db
270316
.select({ value: chat.customizations })
271317
.from(chat)
272-
.where(sql`${chat.customizations}::text LIKE '%Url%'`),
273-
db.select({ value: oauthClient.icon }).from(oauthClient).where(isNotNull(oauthClient.icon)),
318+
.where(matching(chat.customizations, sql`${chat.customizations}::text LIKE '%Url%'`)),
319+
db
320+
.select({ value: oauthClient.icon })
321+
.from(oauthClient)
322+
.where(matching(oauthClient.icon, isNotNull(oauthClient.icon))),
274323
]
275324
for (const source of sources) for (const row of await source) collectStrings(row.value, values)
276325
const organizations = await db
@@ -282,9 +331,9 @@ async function loadUrlReferencedKeys(): Promise<Set<string>> {
282331
.from(organization)
283332
.where(
284333
or(
285-
isNotNull(organization.logo),
286-
isNotNull(organization.whitelabelSettings),
287-
isNotNull(organization.metadata)
334+
matching(organization.logo, isNotNull(organization.logo)),
335+
matching(organization.whitelabelSettings, isNotNull(organization.whitelabelSettings)),
336+
matching(organization.metadata, isNotNull(organization.metadata))
288337
)
289338
)
290339
for (const row of organizations) collectStrings([row.logo, row.whitelabel, row.metadata], values)
@@ -298,16 +347,37 @@ async function loadUrlReferencedKeys(): Promise<Set<string>> {
298347
*
299348
* - a row still names it: `workspace_files.key`, `workspace_file.key`,
300349
* `workspace_file_version.key`, `document.storage_key`, `execution_large_values.key`; or
301-
* - a stored asset URL names it (see {@link loadUrlReferencedKeys}), loaded once per guard.
350+
* - it sits under a {@link PUBLIC_SERVE_PREFIXES} root and a stored asset URL names it (see
351+
* {@link loadUrlReferencedKeys}). Only rows containing the key's longest URL-safe run are read,
352+
* each run at most once per guard. Any other key is unreadable without a row, so no URL column
353+
* is read for it.
302354
*
303355
* Every row of a deleted workspace or account is gone when this runs, so a remaining reference
304356
* belongs to data that outlived the deletion — a moved knowledge base, a handed-off file, an
305357
* organization logo. References embedded in free-form JSON or text — workflow sub-block values,
306358
* chat messages, Markdown links — are not consulted; for the deleted account's own chat
307-
* attachments that is the intended erasure.
359+
* attachments that is the intended erasure. A failed lookup throws, so nothing is deleted.
308360
*/
309361
export function createStorageReferenceGuard(): (keys: readonly string[]) => Promise<Set<string>> {
310-
let urlKeys: Promise<Set<string>> | undefined
362+
const loadedNeedles = new Set<string>()
363+
const named = new Set<string>()
364+
let loadedAll = false
365+
const loadNamedKeys = async (publicKeys: readonly string[]) => {
366+
if (loadedAll || publicKeys.length === 0) return
367+
const needles = publicKeys.map(urlNeedle)
368+
if (needles.includes(null)) {
369+
for (const key of await loadUrlReferencedKeys(null)) named.add(key)
370+
loadedAll = true
371+
return
372+
}
373+
const pending = [...new Set(needles.flatMap((needle) => needle ?? []))].filter(
374+
(needle) => !loadedNeedles.has(needle)
375+
)
376+
for (const batch of chunkArray(pending, NEEDLES_PER_QUERY)) {
377+
for (const key of await loadUrlReferencedKeys(batch)) named.add(key)
378+
for (const needle of batch) loadedNeedles.add(needle)
379+
}
380+
}
311381
return async (keys) => {
312382
const referenced = new Set<string>()
313383
if (keys.length === 0) return referenced
@@ -335,9 +405,11 @@ export function createStorageReferenceGuard(): (keys: readonly string[]) => Prom
335405
.where(inArray(executionLargeValues.key, list))),
336406
]
337407
for (const row of rows) if (row.key) referenced.add(row.key)
338-
urlKeys ??= loadUrlReferencedKeys()
339-
const named = await urlKeys
340-
for (const key of list) if (named.has(key)) referenced.add(key)
408+
const publicKeys = list.filter((key) =>
409+
PUBLIC_SERVE_PREFIXES.some((prefix) => key.startsWith(prefix))
410+
)
411+
await loadNamedKeys(publicKeys)
412+
for (const key of publicKeys) if (named.has(key)) referenced.add(key)
341413
return referenced
342414
}
343415
}
@@ -444,7 +516,11 @@ const sweepWorkspaceStorage: OutboxHandler<unknown> = async (raw, context) => {
444516
const result = await sweepWorkspacePrefix(prefix, storageContext, context, isReferenced)
445517
if (result.deleted > 0) progressed = true
446518
if (!result.complete) {
447-
return progressed && !result.stalled
519+
if (result.stalled)
520+
return deferOutboxHandler(
521+
'Workspace storage sweep removed nothing from a full listing page'
522+
)
523+
return progressed
448524
? continueOutboxHandler('Continuing workspace storage sweep')
449525
: deferOutboxHandler('Workspace storage sweep made no progress before its deadline')
450526
}

0 commit comments

Comments
 (0)