Skip to content

Commit 598f18f

Browse files
committed
fix(uploads): guard logo replacements and reclaim retired images
1 parent de4856a commit 598f18f

7 files changed

Lines changed: 348 additions & 64 deletions

File tree

.github/workflows/test-build.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,7 @@ jobs:
190190
lib/knowledge/__integration__/search-reference-batching.integration.ts
191191
lib/core/outbox/service.integration.ts
192192
lib/knowledge/__integration__/connector-upload.integration.ts
193+
lib/uploads/contexts/organization-logo/application.integration.ts
193194
194195
test-build:
195196
name: Lint and Test
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
/** Real PostgreSQL verifies cross-session registration and durable logo retention. */
2+
import { db } from '@sim/db'
3+
import { withInsertColumns } from '@sim/db/insert-columns'
4+
import { member, organization, organizationColumns, uploadSession } from '@sim/db/schema'
5+
import { generateId } from '@sim/utils/id'
6+
import { eq } from 'drizzle-orm'
7+
import type { Sql } from 'postgres'
8+
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const fixture = vi.hoisted(() => ({
11+
schema: '',
12+
connection: undefined as Sql | undefined,
13+
deleteObject: vi.fn(),
14+
headObject: vi.fn(),
15+
}))
16+
17+
vi.mock('@sim/db', async () => {
18+
const { drizzle } = await import('drizzle-orm/postgres-js')
19+
const { default: postgres } = await import('postgres')
20+
const { withUtcTimestamps } = await import('@sim/db/timestamps')
21+
const { generateId } = await import('@sim/utils/id')
22+
fixture.schema = `logo_test_${generateId().replaceAll('-', '')}`
23+
fixture.connection = postgres(
24+
process.env.KNOWLEDGE_ACL_TEST_DATABASE_URL!,
25+
withUtcTimestamps({
26+
max: 4,
27+
prepare: false,
28+
fetch_types: false,
29+
connection: { search_path: fixture.schema },
30+
onnotice: () => {},
31+
})
32+
)
33+
const database = drizzle(fixture.connection)
34+
return { db: database, dbFor: () => database }
35+
})
36+
37+
vi.mock('@/lib/permission-groups/resolve.server', () => ({
38+
getUserPermissionConfigForOrganization: async () => null,
39+
}))
40+
vi.mock('@sim/audit', async (importOriginal) => ({
41+
...(await importOriginal<typeof import('@sim/audit')>()),
42+
recordAudit: vi.fn(),
43+
}))
44+
vi.mock('@/lib/uploads/upload-session/cleanup', () => ({
45+
maybeCleanupLocalUploadArtifacts: async () => ({ scanned: 0, removed: 0 }),
46+
}))
47+
vi.mock('@/lib/uploads/upload-session/provider', async (importOriginal) => ({
48+
...(await importOriginal<typeof import('@/lib/uploads/upload-session/provider')>()),
49+
createPutProviderTransfer: async () => ({
50+
method: 'put',
51+
url: 'http://localhost/upload',
52+
headers: {},
53+
}),
54+
headProviderObject: fixture.headObject,
55+
deleteProviderObjectVersion: fixture.deleteObject,
56+
}))
57+
58+
import {
59+
createOrganizationLogoUpload,
60+
finalizeOrganizationLogoUpload,
61+
} from '@/lib/uploads/contexts/organization-logo/application'
62+
import { cleanupExpiredUploadSessions } from '@/lib/uploads/upload-session/service'
63+
64+
describe('organization logo concurrency and retention', () => {
65+
const organizationId = generateId()
66+
const principals = [
67+
{ kind: 'session', userId: generateId(), sessionId: generateId() },
68+
{ kind: 'session', userId: generateId(), sessionId: generateId() },
69+
] as const
70+
const request = { headers: new Headers() }
71+
72+
beforeAll(async () => {
73+
const connection = fixture.connection!
74+
await connection`CREATE SCHEMA ${connection(fixture.schema)}`
75+
for (const table of ['user', 'member', 'organization', 'upload_session']) {
76+
await connection`CREATE TABLE ${connection(table)} (LIKE ${connection(`public.${table}`)} INCLUDING ALL)`
77+
}
78+
await db.insert(withInsertColumns(organization, organizationColumns)).values({
79+
id: organizationId,
80+
name: 'Logo test organization',
81+
slug: generateId(),
82+
createdAt: new Date(),
83+
})
84+
await db.insert(member).values(
85+
principals.map((principal) => ({
86+
id: generateId(),
87+
organizationId,
88+
userId: principal.userId,
89+
role: 'admin',
90+
}))
91+
)
92+
})
93+
94+
beforeEach(async () => {
95+
vi.clearAllMocks()
96+
await db.delete(uploadSession)
97+
await db.update(organization).set({ logo: null }).where(eq(organization.id, organizationId))
98+
})
99+
100+
afterAll(async () => {
101+
const connection = fixture.connection
102+
if (!connection) return
103+
try {
104+
await connection`DROP SCHEMA ${connection(fixture.schema)} CASCADE`
105+
} finally {
106+
await connection.end()
107+
}
108+
})
109+
110+
async function start(principal = principals[0]) {
111+
const session = await createOrganizationLogoUpload(principal, {
112+
organizationId,
113+
name: 'logo.png',
114+
contentType: 'image/png',
115+
size: 100,
116+
localOrigin: 'http://localhost',
117+
})
118+
await db
119+
.update(uploadSession)
120+
.set({ status: 'finalizing' })
121+
.where(eq(uploadSession.id, session.id))
122+
return session
123+
}
124+
125+
async function currentLogo() {
126+
const [row] = await db
127+
.select({ logo: organization.logo })
128+
.from(organization)
129+
.where(eq(organization.id, organizationId))
130+
return row.logo
131+
}
132+
133+
it('rejects an older upload after a different administrator completes a newer one', async () => {
134+
const older = await start()
135+
const newer = await start(principals[1])
136+
const result = await finalizeOrganizationLogoUpload(principals[1], newer, request)
137+
await expect(
138+
finalizeOrganizationLogoUpload(principals[0], older, request)
139+
).rejects.toMatchObject({ code: 'conflict' })
140+
expect(await currentLogo()).toBe(result.value.path)
141+
})
142+
143+
it('allows only one concurrent completion from the same starting logo', async () => {
144+
const sessions = await Promise.all(principals.map((principal) => start(principal)))
145+
const results = await Promise.allSettled(
146+
sessions.map((session, index) =>
147+
finalizeOrganizationLogoUpload(principals[index], session, request)
148+
)
149+
)
150+
expect(results.filter((result) => result.status === 'fulfilled')).toHaveLength(1)
151+
const rejected = results.find((result) => result.status === 'rejected')
152+
expect(rejected?.status === 'rejected' && rejected.reason).toMatchObject({ code: 'conflict' })
153+
})
154+
155+
it('retains the active logo and retries deletion of a replaced logo before purging its record', async () => {
156+
const first = await start()
157+
await finalizeOrganizationLogoUpload(principals[0], first, request)
158+
const second = await start(principals[1])
159+
const current = await finalizeOrganizationLogoUpload(principals[1], second, request)
160+
await finalizeOrganizationLogoUpload(principals[0], first, request)
161+
expect(await currentLogo()).toBe(current.value.path)
162+
await db.update(uploadSession).set({
163+
status: 'completed',
164+
completedAt: new Date(Date.now() - 8 * 24 * 60 * 60 * 1000),
165+
})
166+
fixture.headObject.mockImplementation(async ({ key }: { key: string }) => {
167+
expect(key).toBe(first.finalKey)
168+
return {
169+
size: first.fileSize,
170+
contentType: first.contentType,
171+
uploadId: first.id,
172+
version: 'v1',
173+
}
174+
})
175+
fixture.deleteObject.mockRejectedValueOnce(new Error('Storage unavailable'))
176+
expect(await cleanupExpiredUploadSessions()).toEqual({ expired: 0, failed: 1, purged: 0 })
177+
expect(await db.select({ id: uploadSession.id }).from(uploadSession)).toHaveLength(2)
178+
fixture.deleteObject.mockResolvedValue(undefined)
179+
expect(await cleanupExpiredUploadSessions()).toEqual({ expired: 0, failed: 0, purged: 1 })
180+
expect(await db.select({ id: uploadSession.id }).from(uploadSession)).toEqual([
181+
{ id: second.id },
182+
])
183+
expect(await currentLogo()).toBe(current.value.path)
184+
expect(fixture.deleteObject).toHaveBeenLastCalledWith(
185+
expect.objectContaining({
186+
key: first.finalKey,
187+
context: 'organization-logos',
188+
version: 'v1',
189+
})
190+
)
191+
})
192+
})

apps/sim/lib/uploads/contexts/organization-logo/application.test.ts

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,12 @@ const session = {
3939
workspaceId: null,
4040
userId: 'user-1',
4141
metadata: {
42-
organizationLogo: { organizationId: 'org-1', userId: 'user-1', sessionId: 'session-1' },
42+
organizationLogo: {
43+
organizationId: 'org-1',
44+
expectedLogo: null,
45+
userId: 'user-1',
46+
sessionId: 'session-1',
47+
},
4348
},
4449
finalKey: key,
4550
storageKey: key,
@@ -67,12 +72,13 @@ describe('organization logo uploads', () => {
6772
it.each(['owner', 'admin'])(
6873
'allows the current %s and uses their real organization identity',
6974
async (role) => {
70-
dbChainMockFns.limit.mockResolvedValue([{ role }])
75+
dbChainMockFns.limit.mockResolvedValueOnce([{ role }]).mockResolvedValueOnce([{ logo: null }])
7176
await createOrganizationLogoUpload(principal, input)
7277
expect(mocks.create).toHaveBeenCalledWith({
7378
purpose: 'organization_logo',
7479
principal,
7580
organizationId: 'org-1',
81+
expectedLogo: null,
7682
userId: 'user-1',
7783
fileName: 'logo.png',
7884
contentType: 'image/png',
@@ -128,6 +134,7 @@ describe('organization logo uploads', () => {
128134
.mockResolvedValueOnce([{ role: 'admin' }])
129135
.mockResolvedValueOnce([{ completedFileId: null, status: 'finalizing' }])
130136
.mockResolvedValueOnce([{ role: 'admin' }])
137+
.mockResolvedValueOnce([{ logo: null }])
131138
dbChainMockFns.returning
132139
.mockResolvedValueOnce([{ id: 'org-1', name: 'Test org' }])
133140
.mockResolvedValueOnce([{ id: 'upload-1' }])
@@ -136,6 +143,7 @@ describe('organization logo uploads', () => {
136143
expect(dbChainMockFns.set).toHaveBeenCalledWith({ logo: first.value.path })
137144
expect(dbChainMockFns.set).toHaveBeenCalledWith({
138145
completedFileId: 'upload-1',
146+
metadata: { ...session.metadata, organizationLogoPath: first.value.path },
139147
updatedAt: expect.any(Date),
140148
})
141149
expect(recordAudit).toHaveBeenCalledWith(
@@ -176,14 +184,41 @@ describe('organization logo uploads', () => {
176184
.mockResolvedValueOnce([{ role: 'admin' }])
177185
.mockResolvedValueOnce([{ completedFileId: null, status: 'finalizing' }])
178186
.mockResolvedValueOnce([{ role: 'admin' }])
179-
dbChainMockFns.returning.mockResolvedValueOnce([])
187+
.mockResolvedValueOnce([])
180188
await expect(finalizeOrganizationLogoUpload(principal, session, request)).rejects.toMatchObject(
181189
{ code: 'not_found' }
182190
)
183-
expect(dbChainMockFns.set).toHaveBeenCalledOnce()
191+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
192+
expect(recordAudit).not.toHaveBeenCalled()
193+
})
194+
195+
it('rejects an unfinished upload after another session replaces its starting logo', async () => {
196+
dbChainMockFns.limit
197+
.mockResolvedValueOnce([{ role: 'admin' }])
198+
.mockResolvedValueOnce([{ completedFileId: null, status: 'finalizing' }])
199+
.mockResolvedValueOnce([{ role: 'admin' }])
200+
.mockResolvedValueOnce([{ logo: '/api/files/serve/s3/newer-logo.png' }])
201+
await expect(finalizeOrganizationLogoUpload(principal, session, request)).rejects.toMatchObject(
202+
{
203+
code: 'conflict',
204+
}
205+
)
206+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
184207
expect(recordAudit).not.toHaveBeenCalled()
185208
})
186209

210+
it('captures an existing logo as server-authored concurrency state', async () => {
211+
dbChainMockFns.limit
212+
.mockResolvedValueOnce([{ role: 'admin' }])
213+
.mockResolvedValueOnce([{ logo: '/existing-logo.png' }])
214+
await createOrganizationLogoUpload(principal, input)
215+
expect(mocks.create).toHaveBeenCalledWith(
216+
expect.objectContaining({
217+
expectedLogo: '/existing-logo.png',
218+
})
219+
)
220+
})
221+
187222
it('propagates infrastructure errors without reporting a successful logo update', async () => {
188223
dbChainMockFns.limit.mockRejectedValueOnce(new Error('database unavailable'))
189224
await expect(finalizeOrganizationLogoUpload(principal, session, request)).rejects.toThrow(

apps/sim/lib/uploads/contexts/organization-logo/application.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,17 @@ export async function createOrganizationLogoUpload(
3838
input: CreateOrganizationLogoUploadInput
3939
) {
4040
const context = await authorizeOrganizationOperation(principal, organizationLogoOperation, input)
41+
const [current] = await db
42+
.select({ logo: organization.logo })
43+
.from(organization)
44+
.where(eq(organization.id, context.organizationId))
45+
.limit(1)
46+
if (!current) throw new OrchestrationError('not_found', 'Organization not found')
4147
return createUploadSession({
4248
purpose: 'organization_logo',
4349
principal,
4450
organizationId: context.organizationId,
51+
expectedLogo: current.logo,
4552
userId: context.userId,
4653
fileName: input.name,
4754
contentType: input.contentType,
@@ -109,6 +116,19 @@ export async function finalizeOrganizationLogoUpload(
109116
if (!isOrgAdminRole(membership.role)) {
110117
throw new OrchestrationError('forbidden', 'Organization administrator access is required')
111118
}
119+
const [currentOrganization] = await tx
120+
.select({ logo: organization.logo })
121+
.from(organization)
122+
.where(eq(organization.id, binding.organizationId))
123+
.for('update')
124+
.limit(1)
125+
if (!currentOrganization) throw new OrchestrationError('not_found', 'Organization not found')
126+
if (currentOrganization.logo !== binding.expectedLogo) {
127+
throw new OrchestrationError(
128+
'conflict',
129+
'The organization logo changed while this upload was in progress. Please upload it again.'
130+
)
131+
}
112132
const [updated] = await tx
113133
.update(organization)
114134
.set({ logo: value.path })
@@ -117,7 +137,11 @@ export async function finalizeOrganizationLogoUpload(
117137
if (!updated) throw new OrchestrationError('not_found', 'Organization not found')
118138
const [registered] = await tx
119139
.update(uploadSession)
120-
.set({ completedFileId: session.id, updatedAt: new Date() })
140+
.set({
141+
completedFileId: session.id,
142+
metadata: { ...session.metadata, organizationLogoPath: value.path },
143+
updatedAt: new Date(),
144+
})
121145
.where(and(eq(uploadSession.id, session.id), eq(uploadSession.status, 'finalizing')))
122146
.returning({ id: uploadSession.id })
123147
if (!registered) throw new Error('Organization logo registration marker could not be persisted')

apps/sim/lib/uploads/contexts/organization-logo/binding.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { OrchestrationError } from '@/lib/core/orchestration/types'
44

55
export interface OrganizationLogoBinding {
66
organizationId: string
7+
expectedLogo: string | null
78
userId: string
89
sessionId: string
910
}
@@ -27,6 +28,7 @@ export function assertOrganizationLogoControlBinding(
2728
!isRecordLike(binding) ||
2829
typeof binding.organizationId !== 'string' ||
2930
!binding.organizationId ||
31+
(binding.expectedLogo !== null && typeof binding.expectedLogo !== 'string') ||
3032
binding.userId !== session.userId ||
3133
principal.kind !== 'session' ||
3234
principal.userId !== binding.userId ||
@@ -36,6 +38,7 @@ export function assertOrganizationLogoControlBinding(
3638
}
3739
return {
3840
organizationId: binding.organizationId,
41+
expectedLogo: binding.expectedLogo,
3942
userId: principal.userId,
4043
sessionId: principal.sessionId,
4144
}

0 commit comments

Comments
 (0)