Skip to content

Commit 35958a8

Browse files
committed
fix(files): enforce live sharing checks and stabilize browser coverage
1 parent 65e0eeb commit 35958a8

15 files changed

Lines changed: 168 additions & 53 deletions

File tree

‎.github/workflows/checks.yml‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,11 @@ jobs:
203203

204204
- name: Cache Files browser download
205205
if: matrix.group == 'files'
206-
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v5
206+
uses: ./.github/actions/cache
207207
with:
208+
provider: ${{ vars.CI_PROVIDER }}
208209
path: ~/.cache/ms-playwright
209-
key: files-browser-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('bun.lock') }}
210-
restore-keys: files-browser-${{ runner.os }}-${{ runner.arch }}-
210+
key: ${{ github.repository }}-files-browser-${{ github.event_name }}${{ github.event.pull_request.head.repo.fork && '-fork' || '' }}-${{ runner.os }}-${{ runner.arch }}-${{ hashFiles('bun.lock') }}
211211

212212
- name: Install Files browser from the pinned Playwright dependency
213213
if: matrix.group == 'files'

‎apps/sim/app/api/files/public/[token]/content/route.test.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { publicShare } from '@sim/db/schema'
1+
import { publicShare, workspaceFiles } from '@sim/db/schema'
22
import { createRouteContext } from '@sim/testing/helpers/http'
33
import { queueTableRows } from '@sim/testing/mocks/database.mock'
44
import { publicSharesMock, publicSharesMockFns } from '@sim/testing/mocks/public-shares.mock'
@@ -43,7 +43,17 @@ const request = (token = 'tok_1') =>
4343
createMockRequest({ url: `http://localhost/api/files/public/${token}/content` })
4444

4545
const passwordShare = {
46-
share: { id: 'sh_1', token: 'tok_1', authType: 'password', password: 'enc:secret' },
46+
share: {
47+
id: 'sh_1',
48+
token: 'tok_1',
49+
resourceId: 'wf_1',
50+
workspaceId: 'ws-1',
51+
resourceType: 'file',
52+
isActive: true,
53+
authType: 'password',
54+
password: 'enc:secret',
55+
allowedEmails: [],
56+
},
4757
file: {
4858
id: 'wf_1',
4959
key: 'workspace/ws/secret-key.pdf',
@@ -63,6 +73,7 @@ describe('GET /api/files/public/[token]/content', () => {
6373
mockDownloadFile.mockResolvedValue(Buffer.from('data'))
6474
mockResolveServableDoc.mockResolvedValue({ kind: 'passthrough' })
6575
queueTableRows(publicShare, [{ id: 'sh_1' }])
76+
queueTableRows(workspaceFiles, [passwordShare.file])
6677
})
6778

6879
it('returns 401 and never reads storage when a password share is unauthorized', async () => {

‎apps/sim/app/api/files/public/[token]/inline/route.test.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { publicShare } from '@sim/db/schema'
1+
import { publicShare, workspaceFiles } from '@sim/db/schema'
22
import { createRouteContext } from '@sim/testing/helpers/http'
33
import { queueTableRows } from '@sim/testing/mocks/database.mock'
44
import { publicSharesMock, publicSharesMockFns } from '@sim/testing/mocks/public-shares.mock'
@@ -40,7 +40,17 @@ const req = (q: string) =>
4040
createMockRequest({ url: `http://localhost/api/files/public/${TOKEN}/inline?${q}` })
4141

4242
const share = {
43-
share: { id: 'sh_1', token: TOKEN, authType: 'public' },
43+
share: {
44+
id: 'sh_1',
45+
token: TOKEN,
46+
resourceId: 'wf_doc',
47+
workspaceId: 'ws-1',
48+
resourceType: 'file',
49+
isActive: true,
50+
authType: 'public',
51+
password: null,
52+
allowedEmails: [],
53+
},
4454
file: { id: 'wf_doc', key: DOC_KEY, workspaceId: 'ws-1', originalName: 'doc.md' },
4555
workspaceName: 'Acme',
4656
ownerName: 'Jane',
@@ -64,6 +74,7 @@ describe('GET /api/files/public/[token]/inline', () => {
6474
})
6575
mockDownloadFile.mockImplementation(downloadByKey())
6676
queueTableRows(publicShare, [{ id: 'sh_1' }])
77+
queueTableRows(workspaceFiles, [share.file])
6778
})
6879

6980
it('rejects exhausted image budgets before share lookup, authentication, or storage reads', async () => {

‎apps/sim/app/api/files/public/[token]/route.test.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { publicShare as publicShareTable } from '@sim/db/schema'
1+
import { publicShare as publicShareTable, workspaceFiles } from '@sim/db/schema'
22
import { queueTableRows } from '@sim/testing/mocks/database.mock'
33
import { publicSharesMock, publicSharesMockFns } from '@sim/testing/mocks/public-shares.mock'
44
import { NextRequest } from 'next/server'
@@ -40,7 +40,17 @@ const postRequest = (password: string, token = 'tok_1') =>
4040
})
4141

4242
const publicShare = {
43-
share: { id: 'sh_1', token: 'tok_1', authType: 'public', password: null },
43+
share: {
44+
id: 'sh_1',
45+
token: 'tok_1',
46+
resourceId: 'wf_1',
47+
workspaceId: 'ws-secret',
48+
resourceType: 'file',
49+
isActive: true,
50+
authType: 'public',
51+
password: null,
52+
allowedEmails: [],
53+
},
4454
file: {
4555
id: 'wf_1',
4656
key: 'workspace/ws/secret-key.pdf',
@@ -55,14 +65,15 @@ const publicShare = {
5565

5666
const passwordShare = {
5767
...publicShare,
58-
share: { id: 'sh_1', token: 'tok_1', authType: 'password', password: 'enc:secret' },
68+
share: { ...publicShare.share, authType: 'password', password: 'enc:secret' },
5969
}
6070

6171
describe('GET /api/files/public/[token]', () => {
6272
beforeEach(() => {
6373
mockEnforceRateLimit.mockResolvedValue(null) // allow by default
6474
mockValidateDeploymentAuth.mockResolvedValue({ authorized: true }) // public by default
6575
queueTableRows(publicShareTable, [{ id: 'sh_1' }])
76+
queueTableRows(workspaceFiles, [publicShare.file])
6677
})
6778

6879
it('returns 429 when the per-IP rate limit is exceeded', async () => {

‎apps/sim/app/workspace/[workspaceId]/components/resource/components/resource-header/resource-header.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ export const ResourceHeader = memo(function ResourceHeader({
264264
drag={crumbDrag}
265265
/>
266266
)}
267-
{crumb.navigationItems !== undefined && (
267+
{crumb.navigationItems !== undefined && !crumb.editing?.isEditing && (
268268
<BreadcrumbNavigation
269269
label={crumb.label}
270270
items={crumb.navigationItems}

‎apps/sim/lib/api/contracts/tools/file.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
import { z } from 'zod'
2-
import { privateSecretProvenanceBundleSchema } from '@/lib/api/contracts/primitives'
3-
import { shareAuthTypeSchema, upsertFileShareBodySchema } from '@/lib/api/contracts/public-shares'
2+
import {
3+
orExactEnvironmentReference,
4+
privateSecretProvenanceBundleSchema,
5+
} from '@/lib/api/contracts/primitives'
6+
import {
7+
shareAuthTypeSchema,
8+
sharePasswordSchema,
9+
upsertFileShareBodySchema,
10+
} from '@/lib/api/contracts/public-shares'
411
import { toolJsonResponseSchema } from '@/lib/api/contracts/tools/media/shared'
512
import { defineRouteContract } from '@/lib/api/contracts/types'
613
import {
@@ -169,7 +176,7 @@ const fileManageFolderSharingBodySchema = z.object({
169176
path: v2NonRootFolderPathInputSchema,
170177
isActive: z.boolean({ error: 'isActive is required for manage_folder_sharing operation' }),
171178
authType: shareAuthTypeSchema.optional(),
172-
password: z.string().min(1).max(1024).optional(),
179+
password: orExactEnvironmentReference(sharePasswordSchema).optional(),
173180
allowedEmails: upsertFileShareBodySchema.shape.allowedEmails,
174181
})
175182

‎apps/sim/lib/public-shares/folder-reader.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,23 +76,27 @@ export async function resolveSharedFile(
7676
fileId?: string
7777
): Promise<WorkspaceFileRow | null> {
7878
if (!(await shareIsCurrent(resolved))) return null
79-
if (resolved.kind === 'file') {
80-
return fileId === undefined || fileId === resolved.file.id ? resolved.file : null
81-
}
82-
if (!fileId) return null
79+
const targetId = resolved.kind === 'file' ? resolved.share.resourceId : fileId
80+
if (!targetId || (resolved.kind === 'file' && fileId !== undefined && fileId !== targetId))
81+
return null
8382
const [file] = await db
8483
.select()
8584
.from(workspaceFiles)
8685
.where(
8786
and(
88-
eq(workspaceFiles.id, fileId),
87+
eq(workspaceFiles.id, targetId),
8988
eq(workspaceFiles.workspaceId, resolved.share.workspaceId),
9089
eq(workspaceFiles.context, 'workspace'),
9190
isNull(workspaceFiles.deletedAt)
9291
)
9392
)
9493
.limit(1)
95-
if (!file?.folderId || !(await folderBreadcrumbs(resolved, file.folderId))) return null
94+
if (!file) return null
95+
if (
96+
resolved.kind === 'folder' &&
97+
(!file.folderId || !(await folderBreadcrumbs(resolved, file.folderId)))
98+
)
99+
return null
96100
return file
97101
}
98102

‎apps/sim/lib/public-shares/folders.integration.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,4 +228,53 @@ describe('public folder capabilities in PostgreSQL', () => {
228228
.where(eq(publicShare.id, directShareId))
229229
}
230230
})
231+
232+
it.each([
233+
{ name: 'deleted', update: { deletedAt: new Date() } },
234+
{ name: 'moved out of workspace storage', update: { context: 'chat' } },
235+
{
236+
name: 'moved to another workspace',
237+
update: { workspaceId: otherWorkspaceId, folderId: null },
238+
},
239+
])('denies a direct file that is $name during authorization', async ({ update }) => {
240+
try {
241+
await expect(
242+
readPublicSharedFile({
243+
token: directToken,
244+
authorize: async () => {
245+
await db.update(workspaceFiles).set(update).where(eq(workspaceFiles.id, fileId))
246+
return { authorized: true }
247+
},
248+
})
249+
).rejects.toMatchObject({ status: 404 })
250+
} finally {
251+
await db
252+
.update(workspaceFiles)
253+
.set({ deletedAt: null, context: 'workspace', workspaceId, folderId: childId })
254+
.where(eq(workspaceFiles.id, fileId))
255+
}
256+
})
257+
258+
it('reads the current direct file metadata and storage key after authorization', async () => {
259+
const updatedKey = `workspace/${workspaceId}/${generateId()}`
260+
try {
261+
const result = await readPublicSharedFile({
262+
token: directToken,
263+
authorize: async () => {
264+
await db
265+
.update(workspaceFiles)
266+
.set({ originalName: 'updated.txt', key: updatedKey })
267+
.where(eq(workspaceFiles.id, fileId))
268+
return { authorized: true }
269+
},
270+
})
271+
expect(result.file.originalName).toBe('updated.txt')
272+
expect(result.file.key).toBe(updatedKey)
273+
} finally {
274+
await db
275+
.update(workspaceFiles)
276+
.set({ originalName: 'nested.txt', key: `workspace/${workspaceId}/${fileId}` })
277+
.where(eq(workspaceFiles.id, fileId))
278+
}
279+
})
231280
})

‎apps/sim/lib/workspace-files/upload-folders.integration.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { db } from '@sim/db'
22
import { folder } from '@sim/db/schema'
33
import { eq, sql } from 'drizzle-orm'
44
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
5+
import { MAX_FOLDERS_PER_WORKSPACE } from '@/lib/folders/constants'
56
import { prepareUploadFolders } from '@/lib/workspace-files/upload-folders'
67

78
const connection = vi.hoisted(() => ({ close: async () => {} }))
@@ -125,7 +126,7 @@ describe('folder upload preparation in Postgres', () => {
125126
await db.execute(sql`
126127
INSERT INTO folder (id, workspace_id, user_id, resource_type, name)
127128
SELECT 'existing-' || position, 'workspace', 'user', 'file', 'Existing ' || position
128-
FROM generate_series(1, 9999) AS series(position)
129+
FROM generate_series(1, ${MAX_FOLDERS_PER_WORKSPACE - 1}) AS series(position)
129130
`)
130131
await expect(prepare([['Incoming'], ['Incoming', 'Child']])).rejects.toMatchObject({
131132
code: 'conflict',

‎apps/sim/scripts/files-e2e/browser.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,8 @@ export async function dropPaths(page: Page, paths: string[]) {
1212
await session.detach()
1313
}
1414
}
15+
16+
/** Locates the upload menu by its accessible state without matching the empty-state upload action. */
17+
export function uploadMenuTrigger(page: Page, name: string | RegExp = 'Upload') {
18+
return page.getByRole('button', { name, exact: true }).and(page.locator('[aria-haspopup="menu"]'))
19+
}

0 commit comments

Comments
 (0)