fix(downloads): harden active target folders (#507)

This commit is contained in:
Jasper Van
2026-07-17 23:44:17 -04:00
committed by GitHub
parent 15189c4107
commit 882f38e631
8 changed files with 178 additions and 29 deletions
+2 -2
View File
@@ -233,8 +233,8 @@ export function createDownloadTaskRepo(db: Database): DownloadTaskRepo {
notInArray(downloadTasks.status, ['completed', 'failed', 'canceled']),
ne(downloadTasks.targetFolder, ''),
or(
eq(downloadTasks.targetFolder, folderPath),
sql`substr(${downloadTasks.targetFolder}, 1, length(${prefix})) = ${prefix}`,
sql`lower(${downloadTasks.targetFolder}) = lower(${folderPath})`,
sql`lower(substr(${downloadTasks.targetFolder}, 1, length(${prefix}))) = lower(${prefix})`,
),
),
)
@@ -735,6 +735,14 @@ describe('Download tasks API integration', () => {
`)
expect(liveTargets[0].count).toBe(1)
// Multi-file downloads write below the configured target. Recreate a child
// directory too if it disappears between two uploaded files.
await db.run(sql`
UPDATE matters
SET trashed_at = ${Date.now()}
WHERE id = ${folder.id}
`)
const createNestedObjectRes = await app.request('/api/objects', {
method: 'POST',
headers: uploadHeaders,
@@ -753,6 +761,16 @@ describe('Download tasks API integration', () => {
}
expect(nestedObject.status).toBe('draft')
expect(nestedObject.upload.urls).toEqual(['https://presigned-upload.example.com'])
const liveNestedParents = await db.all<{ count: number }>(sql`
SELECT count(*) AS count
FROM matters
WHERE org_id = ${createdTask.orgId}
AND parent = 'Remote Downloads'
AND name = 'fixture-dir'
AND status = 'active'
AND trashed_at IS NULL
`)
expect(liveNestedParents[0].count).toBe(1)
// Finalize the nested upload via the completions endpoint (single-PUT path).
const nestedConfirmRes = await app.request(
@@ -1449,15 +1467,18 @@ describe('Download tasks API integration', () => {
headers: { ...user, 'Content-Type': 'application/json' },
body: JSON.stringify({
source: { type: 'http', uri: 'https://example.com/fixture.txt' },
targetFolder: 'media/Movies',
targetFolder: 'Media/Movies',
}),
})
expect(createTaskRes.status).toBe(201)
const task = (await createTaskRes.json()) as DownloadTask
// Tasks created before canonical target persistence can differ only in case
// from the live matter path. They must still hold the directory lease.
await db.run(sql`UPDATE download_tasks SET target_folder = 'media/movies' WHERE id = ${task.id}`)
const folders = await db.all<{ id: string; name: string }>(sql`
SELECT id, name FROM matters WHERE org_id = ${task.orgId} AND dirtype = 1
`)
const media = folders.find((folder) => folder.name === 'media')
const media = folders.find((folder) => folder.name === 'Media')
const movies = folders.find((folder) => folder.name === 'Movies')
expect(media).toBeTruthy()
expect(movies).toBeTruthy()
@@ -1469,7 +1490,7 @@ describe('Download tasks API integration', () => {
}
expect(deleteBody.error.details[0]).toMatchObject({
reason: 'DIRECTORY_IN_USE',
metadata: { taskId: task.id, targetFolder: 'media/Movies' },
metadata: { taskId: task.id, targetFolder: 'media/movies' },
})
const renameTarget = await app.request(`/api/objects/${movies?.id}`, {
@@ -1484,6 +1505,61 @@ describe('Download tasks API integration', () => {
expect(deleteAfterCompletion.status).toBe(204)
})
it('recreates a missing target when retrying or restarting a task', async () => {
const { app, db } = await createTestApp()
await insertStorage(db)
const user = await authedHeaders(app, 'reactivate-target-user@example.com')
const createRes = await app.request('/api/downloads/tasks', {
method: 'POST',
headers: { ...user, 'Content-Type': 'application/json' },
body: JSON.stringify({
source: { type: 'http', uri: 'https://example.com/reactivate.bin' },
targetFolder: 'Archive/Movies',
}),
})
const task = (await createRes.json()) as DownloadTask
const removeTarget = () =>
db.run(sql`
UPDATE matters
SET trashed_at = ${Date.now()}
WHERE org_id = ${task.orgId} AND parent = 'Archive' AND name = 'Movies' AND trashed_at IS NULL
`)
await removeTarget()
await db.run(sql`UPDATE download_tasks SET status = 'failed' WHERE id = ${task.id}`)
const retryRes = await app.request(`/api/downloads/tasks/${task.id}/attempts`, {
method: 'POST',
headers: { ...user, 'Content-Type': 'application/json' },
body: JSON.stringify({ fresh: false }),
})
expect(retryRes.status).toBe(201)
await expect(retryRes.json()).resolves.toMatchObject({ status: { state: 'queued' } })
await removeTarget()
const restartRes = await app.request(`/api/downloads/tasks/${task.id}/attempts`, {
method: 'POST',
headers: { ...user, 'Content-Type': 'application/json' },
body: JSON.stringify({ fresh: true }),
})
expect(restartRes.status).toBe(201)
await expect(restartRes.json()).resolves.toMatchObject({
spec: { destination: { folder: 'Archive/Movies' } },
status: { state: 'queued', attempt: 2 },
})
const liveTargets = await db.all<{ count: number }>(sql`
SELECT count(*) AS count
FROM matters
WHERE org_id = ${task.orgId}
AND parent = 'Archive'
AND name = 'Movies'
AND status = 'active'
AND trashed_at IS NULL
`)
expect(liveTargets[0].count).toBe(1)
})
it('returns storage failure details when multipart upload completion fails [spec: download-tasks/upload-completion-failure]', async () => {
vi.mocked(S3Service.prototype.completeMultipartUpload).mockRejectedValueOnce(new Error('InvalidPart: part missing'))
const { app, db } = await createTestApp()
@@ -3,7 +3,7 @@ import { describe, expect, it, vi } from 'vitest'
import type { Deps } from '../deps'
import type { Matter, MatterRepo, StorageRepo } from '../ports'
import { NameConflictError } from '../ports'
import { assertFolderNotUsedByDownload, ensureDownloadTargetFolder } from './download-folders'
import { assertFolderNotUsedByDownload, ensureDownloadFolderPath } from './download-folders'
const folder = (name: string, parent = ''): Matter => ({
id: `${parent}/${name}`,
@@ -45,9 +45,9 @@ describe('download target folders', () => {
const findActiveConflict = vi.fn()
const select = vi.fn()
await expect(
ensureDownloadTargetFolder(ensureDeps({ findActiveConflict, select }), {
ensureDownloadFolderPath(ensureDeps({ findActiveConflict, select }), {
orgId: 'org-1',
targetFolder: '',
folderPath: '',
actorId: 'user-1',
}),
).resolves.toBe('')
@@ -62,9 +62,9 @@ describe('download target folders', () => {
})
await expect(
ensureDownloadTargetFolder(ensureDeps({ findActiveConflict, create }), {
ensureDownloadFolderPath(ensureDeps({ findActiveConflict, create }), {
orgId: 'org-1',
targetFolder: 'Downloads',
folderPath: 'Downloads',
actorId: 'user-1',
}),
).resolves.toBe('Downloads')
@@ -77,9 +77,9 @@ describe('download target folders', () => {
})
await expect(
ensureDownloadTargetFolder(ensureDeps({ findActiveConflict, create }), {
ensureDownloadFolderPath(ensureDeps({ findActiveConflict, create }), {
orgId: 'org-1',
targetFolder: 'Downloads',
folderPath: 'Downloads',
actorId: 'user-1',
}),
).resolves.toBe('Downloads')
@@ -88,7 +88,7 @@ describe('download target folders', () => {
it('preserves unexpected create failures and unresolved conflicts', async () => {
const unexpected = new Error('database unavailable')
await expect(
ensureDownloadTargetFolder(
ensureDownloadFolderPath(
ensureDeps({
create: async () => {
throw unexpected
@@ -96,7 +96,7 @@ describe('download target folders', () => {
}),
{
orgId: 'org-1',
targetFolder: 'Downloads',
folderPath: 'Downloads',
actorId: 'user-1',
},
),
@@ -104,7 +104,7 @@ describe('download target folders', () => {
const conflict = new NameConflictError('Downloads', 'winner')
await expect(
ensureDownloadTargetFolder(
ensureDownloadFolderPath(
ensureDeps({
create: async () => {
throw conflict
@@ -112,7 +112,7 @@ describe('download target folders', () => {
}),
{
orgId: 'org-1',
targetFolder: 'Downloads',
folderPath: 'Downloads',
actorId: 'user-1',
},
),
@@ -122,9 +122,9 @@ describe('download target folders', () => {
it('rejects a file in the target path', async () => {
const file = { ...folder('Downloads'), type: 'text/plain', dirtype: DirType.FILE }
await expect(
ensureDownloadTargetFolder(ensureDeps({ findActiveConflict: async () => file }), {
ensureDownloadFolderPath(ensureDeps({ findActiveConflict: async () => file }), {
orgId: 'org-1',
targetFolder: 'Downloads/Movies',
folderPath: 'Downloads/Movies',
actorId: 'user-1',
}),
).rejects.toMatchObject({
@@ -14,11 +14,11 @@ function targetIsFile(path: string): AppError {
})
}
export async function ensureDownloadTargetFolder(
export async function ensureDownloadFolderPath(
deps: Pick<Deps, 'matter' | 'storages'>,
params: { orgId: string; targetFolder: string; actorId: string },
params: { orgId: string; folderPath: string; actorId: string },
): Promise<string> {
const parts = params.targetFolder.split('/').filter(Boolean)
const parts = params.folderPath.split('/').filter(Boolean)
if (parts.length === 0) return ''
let parent = ''
+17 -3
View File
@@ -31,7 +31,7 @@ import type {
} from '../ports'
import { DownloadError, featureBlocked } from '../ports'
import { loadBindingState } from '../site/licensing'
import { ensureDownloadTargetFolder } from './download-folders'
import { ensureDownloadFolderPath } from './download-folders'
import { RemoteDownloadBillingBlockedError, reportRemoteDownloadUnit } from './remote-download-usage'
// Pure orchestration over the downloader / download-task repos: registration,
@@ -257,9 +257,9 @@ export async function createDownloadTask(
userId: string,
input: CreateDownloadTaskInput,
): Promise<DownloadTask> {
const targetFolder = await ensureDownloadTargetFolder(deps, {
const targetFolder = await ensureDownloadFolderPath(deps, {
orgId,
targetFolder: input.targetFolder,
folderPath: input.targetFolder,
actorId: userId,
})
const now = new Date()
@@ -542,7 +542,9 @@ export async function performDownloadTaskAction(
if (!['paused', 'suspended'].includes(task.status)) {
throw new DownloadError('invalid_state', 'Only paused or suspended tasks can be resumed')
}
const targetFolder = await ensureTaskTargetFolder(deps, task)
await deps.downloadTasks.setFields(id, {
targetFolder,
status: 'queued',
assignedDownloaderId: null,
assignedAt: null,
@@ -580,7 +582,9 @@ export async function performDownloadTaskAction(
if (task.status !== 'failed') {
throw new DownloadError('invalid_state', 'Only failed tasks can be retried')
}
const targetFolder = await ensureTaskTargetFolder(deps, task)
await deps.downloadTasks.setFields(id, {
targetFolder,
status: 'queued',
assignedDownloaderId: null,
errorCode: null,
@@ -600,7 +604,9 @@ export async function performDownloadTaskAction(
if (!RESTARTABLE_TASK_STATUSES.includes(task.status as (typeof RESTARTABLE_TASK_STATUSES)[number])) {
throw new DownloadError('invalid_state', 'Only inactive tasks can be restarted')
}
const targetFolder = await ensureTaskTargetFolder(deps, task)
await deps.downloadTasks.setFields(id, {
targetFolder,
status: 'queued',
assignedDownloaderId: null,
attempt: task.attempt + 1,
@@ -624,6 +630,14 @@ export async function performDownloadTaskAction(
throw new DownloadError('invalid_state')
}
async function ensureTaskTargetFolder(deps: DownloadsDeps, task: DownloadTaskRecord): Promise<string> {
return ensureDownloadFolderPath(deps, {
orgId: task.orgId,
folderPath: task.targetFolder,
actorId: task.createdByUserId,
})
}
export async function assertTaskUploadAllowed(
deps: DownloadsDeps,
params: { taskId: string; downloaderId: string },
+53
View File
@@ -488,6 +488,37 @@ describe('object usecase', () => {
// object key uses the creator's uid
expect(arg.object).toContain('o1/creator/')
})
it('recreates the actual upload parent and uses its canonical path', async () => {
const create = vi.fn(async (input: Parameters<MatterRepo['create']>[0]) =>
input.dirtype === DirType.USER_FOLDER
? folder(input.name, { name: input.name, parent: input.parent })
: file('m', input as Partial<Matter>),
)
const { deps } = makeDeps({
matter: {
findActiveConflict: async (_orgId, parent, name) =>
parent === '' && name === 'inbox' ? folder('Inbox', { name: 'Inbox' }) : null,
create,
},
})
const out = await createObject(deps, {
orgId: 'o1',
actor: {
kind: 'download-task-upload',
downloaderId: 'd1',
taskId: 't1',
targetFolder: 'inbox',
createdByUserId: 'creator',
},
input: { name: 'part.txt', type: 'text/plain', dirtype: DirType.FILE, parent: 'inbox/movie' },
})
expect(out.ok).toBe(true)
expect(create).toHaveBeenCalledWith(expect.objectContaining({ name: 'movie', parent: 'Inbox' }))
expect(create).toHaveBeenLastCalledWith(expect.objectContaining({ name: 'part.txt', parent: 'Inbox/movie' }))
})
})
// An active upload session record as the repo returns it. uploadId=null is a
@@ -1071,6 +1102,28 @@ describe('object usecase', () => {
expectError(out, 403, 'Forbidden')
})
it('rejects moving a folder used by an active download before copying it', async () => {
const copyObject = vi.fn(async () => {})
const { deps } = makeDeps({
matter: { get: async () => folder('Downloads', { name: 'Downloads', status: 'active' }) },
downloadTasks: {
findActiveTargetWithin: async () =>
({ id: 'task-1', targetFolder: 'Downloads/Movies' }) as unknown as DownloadTaskRecord,
},
s3: { copyObject },
})
await expect(
transferObject(deps, {
orgId: 'o1',
userId: 'u1',
objectId: 'downloads',
input: { targetOrgId: 'o2', targetParent: '', mode: 'move' },
}),
).rejects.toMatchObject({ httpStatus: 409, meta: { reason: 'DIRECTORY_IN_USE' } })
expect(copyObject).not.toHaveBeenCalled()
})
it('rejects a transfer that exceeds the target quota', async () => {
const { deps } = makeDeps({
matter: { get: async () => file('m1', { status: 'active' }) },
+10 -5
View File
@@ -22,7 +22,7 @@ import type {
import type { ObjectUploadInstructions } from '@shared/types'
import { buildObjectKey, fileExt } from '../lib/path-template'
import type { Deps } from './deps'
import { assertFolderNotUsedByDownload, ensureDownloadTargetFolder } from './downloads/download-folders'
import { assertFolderNotUsedByDownload, ensureDownloadFolderPath } from './downloads/download-folders'
import { assertTaskUploadAllowed } from './downloads/downloads'
import {
type ActivityRepo,
@@ -159,7 +159,8 @@ export async function createObject(
params: { orgId: string; actor: ObjectActor; input: CreateMatterInput },
): Promise<CreateObjectOutcome> {
const { orgId, actor, input } = params
const { name, type, parent, dirtype, onConflict } = input
const { name, type, dirtype, onConflict } = input
let { parent } = input
const isFolder = dirtype !== DirType.FILE
const size = input.size ?? 0
@@ -171,9 +172,9 @@ export async function createObject(
return { ok: false, error: forbidden('Target folder is outside task authorization') }
}
await assertTaskUploadAllowed(deps as Deps, { taskId: actor.taskId, downloaderId: actor.downloaderId })
await ensureDownloadTargetFolder(deps, {
parent = await ensureDownloadFolderPath(deps, {
orgId,
targetFolder: actor.targetFolder,
folderPath: parent,
actorId: actor.createdByUserId,
})
}
@@ -729,7 +730,10 @@ export type TransferObjectResult = Awaited<ReturnType<typeof copyMatterToOrg>> &
export type TransferObjectOutcome = { ok: true; result: TransferObjectResult } | { ok: false; error: AppError }
export async function transferObject(
deps: Pick<Deps, 'matter' | 'storages' | 's3' | 'quota' | 'storageUsage' | 'share' | 'org' | 'activity'>,
deps: Pick<
Deps,
'matter' | 'storages' | 's3' | 'quota' | 'storageUsage' | 'share' | 'org' | 'activity' | 'downloadTasks'
>,
params: { orgId: string; userId: string; objectId: string; input: TransferMatterInput },
): Promise<TransferObjectOutcome> {
const { orgId, userId, objectId, input } = params
@@ -744,6 +748,7 @@ export async function transferObject(
if (mode === 'move' && !(await hasEditorAccess(deps, { orgId, userId }))) {
return { ok: false, error: forbidden() }
}
if (mode === 'move') await assertFolderNotUsedByDownload(deps, { orgId, folder: source })
if (!(await deps.org.canWriteToOrg(userId, targetOrgId))) {
return { ok: false, error: forbidden() }
}
+1
View File
@@ -206,6 +206,7 @@ export interface DownloadTaskRepo {
}
export interface UpdateDownloadTaskFields {
targetFolder?: string
status?: string
assignedDownloaderId?: string | null
attempt?: number