Skip to content

Commit e69a687

Browse files
johavenjohaven
committed
feat(backend:files): improve file upload handling with comprehensive overwrite support and directory conflict resolution
1 parent 3b734b8 commit e69a687

File tree

7 files changed

+39
-19
lines changed

7 files changed

+39
-19
lines changed

‎backend/src/applications/files/files.controller.spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,10 @@ describe(FilesController.name, () => {
103103
expect(filesMethodsMock.make).toHaveBeenCalledWith(fakeUser, fakeSpace, dto)
104104
})
105105

106-
it('upload() should call filesMethods.upload(req, res)', async () => {
107-
await filesController.uploadCreate(fakeReq, fakeRes)
106+
it('upload() should call filesMethods.upload(req)', async () => {
107+
await filesController.uploadCreate(fakeReq)
108108

109-
expect(filesMethodsMock.upload).toHaveBeenCalledWith(fakeReq, fakeRes)
109+
expect(filesMethodsMock.upload).toHaveBeenCalledWith(fakeReq)
110110
})
111111

112112
it('copy() should call filesMethods.copy(user, space, dto) and return its result', async () => {

‎backend/src/applications/files/files.controller.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,13 @@ export class FilesController {
7474
}
7575

7676
@Post(`${FILES_ROUTE.OPERATION}/${FILE_OPERATION.UPLOAD}/*`)
77-
async uploadCreate(@Req() req: FastifySpaceRequest, @Res({ passthrough: true }) res: FastifyReply): Promise<void> {
78-
return this.filesMethods.upload(req, res)
77+
async uploadCreate(@Req() req: FastifySpaceRequest): Promise<void> {
78+
return this.filesMethods.upload(req)
7979
}
8080

8181
@Put(`${FILES_ROUTE.OPERATION}/${FILE_OPERATION.UPLOAD}/*`)
82-
async uploadOverwrite(@Req() req: FastifySpaceRequest, @Res({ passthrough: true }) res: FastifyReply): Promise<void> {
83-
return this.filesMethods.upload(req, res)
82+
async uploadOverwrite(@Req() req: FastifySpaceRequest): Promise<void> {
83+
return this.filesMethods.upload(req)
8484
}
8585

8686
@Copy(`${FILES_ROUTE.OPERATION}/*`)

‎backend/src/applications/files/services/files-manager.service.spec.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import { HttpService } from '@nestjs/axios'
88
import { Test, TestingModule } from '@nestjs/testing'
99
import { DB_TOKEN_PROVIDER } from '../../../infrastructure/database/constants'
10+
import { SpacesManager } from '../../spaces/services/spaces-manager.service'
1011
import { FilesLockManager } from './files-lock-manager.service'
1112
import { FilesManager } from './files-manager.service'
1213
import { FilesQueries } from './files-queries.service'
@@ -19,6 +20,7 @@ describe(FilesManager.name, () => {
1920
providers: [
2021
{ provide: DB_TOKEN_PROVIDER, useValue: {} },
2122
{ provide: FilesLockManager, useValue: {} },
23+
{ provide: SpacesManager, useValue: {} },
2224
{
2325
provide: HttpService,
2426
useValue: {}

‎backend/src/applications/files/services/files-manager.service.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,14 @@ import { AxiosResponse } from 'axios'
1111
import fs from 'node:fs'
1212
import path from 'node:path'
1313
import { Readable } from 'node:stream'
14-
import { pipeline } from 'node:stream/promises'
1514
import { extract as extractTar } from 'tar'
1615
import { FastifyAuthenticatedRequest } from '../../../authentication/interfaces/auth-request.interface'
1716
import { generateThumbnail } from '../../../common/image'
1817
import { HTTP_METHOD } from '../../applications.constants'
1918
import { SPACE_OPERATION } from '../../spaces/constants/spaces'
2019
import { FastifySpaceRequest } from '../../spaces/interfaces/space-request.interface'
2120
import { SpaceEnv } from '../../spaces/models/space-env.model'
21+
import { SpacesManager } from '../../spaces/services/spaces-manager.service'
2222
import { realTrashPathFromSpace } from '../../spaces/utils/paths'
2323
import { canAccessToSpace, haveSpaceEnvPermissions } from '../../spaces/utils/permissions'
2424
import { UserModel } from '../../users/models/user.model'
@@ -68,7 +68,8 @@ export class FilesManager {
6868
constructor(
6969
private readonly http: HttpService,
7070
private readonly filesQueries: FilesQueries,
71-
private readonly filesLockManager: FilesLockManager
71+
private readonly filesLockManager: FilesLockManager,
72+
private readonly spacesManager: SpacesManager
7273
) {}
7374

7475
sendFileFromSpace(space: SpaceEnv, asAttachment = false, downloadName = ''): SendFile {
@@ -181,9 +182,10 @@ export class FilesManager {
181182
POST: create new resource
182183
PUT: create or update new resource (even if a parent path does not exist)
183184
*/
185+
const overwrite = req.method === HTTP_METHOD.PUT
184186
const realParentPath = dirName(space.realPath)
185187

186-
if (req.method === HTTP_METHOD.POST) {
188+
if (!overwrite) {
187189
if (await isPathExists(space.realPath)) {
188190
throw new FileError(HttpStatus.BAD_REQUEST, 'Resource already exists')
189191
}
@@ -198,14 +200,31 @@ export class FilesManager {
198200
const basePath = realParentPath + path.sep
199201

200202
for await (const part of req.files()) {
201-
// part.filename may contain a path like foo/bar.txt.
203+
// part.filename may contain a path like foo/bar.txt
202204
const dstFile = path.resolve(basePath, part.filename)
203205
// prevent path traversal
204206
if (!dstFile.startsWith(basePath)) {
205207
throw new FileError(HttpStatus.FORBIDDEN, 'Location is not allowed')
206208
}
207-
// make dir in space
209+
208210
const dstDir = dirName(dstFile)
211+
212+
if (overwrite) {
213+
// prevent errors when an uploaded file would replace a directory with the same name
214+
// only applies in `overwrite` cases
215+
if ((await isPathExists(dstFile)) && (await isPathIsDir(dstFile))) {
216+
// If a directory already exists at the destination path, delete it to allow overwriting with the uploaded file
217+
const dstUrl = path.join(path.dirname(space.url), part.filename)
218+
const dstSpace = await this.spacesManager.spaceEnv(user, dstUrl.split('/'))
219+
await this.delete(user, dstSpace)
220+
} else if ((await isPathExists(dstDir)) && !(await isPathIsDir(dstDir))) {
221+
// If the destination's parent exists but is a file, remove it so we can create the directory
222+
const dstUrl = path.join(path.dirname(space.url), path.dirname(part.filename))
223+
const dstSpace = await this.spacesManager.spaceEnv(user, dstUrl.split('/'))
224+
await this.delete(user, dstSpace)
225+
}
226+
}
227+
// make dir in space
209228
if (!(await isPathExists(dstDir))) {
210229
await makeDir(dstDir, true)
211230
}
@@ -215,7 +234,7 @@ export class FilesManager {
215234
if (!ok) throw new LockConflict(fileLock, 'Conflicting lock')
216235
// do
217236
try {
218-
await pipeline(part.file, fs.createWriteStream(dstFile, { highWaterMark: DEFAULT_HIGH_WATER_MARK }))
237+
await writeFromStream(dstFile, part.file)
219238
} finally {
220239
await this.filesLockManager.removeLock(fileLock.key)
221240
}

‎backend/src/applications/files/services/files-methods.service.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,15 +39,11 @@ export class FilesMethods {
3939
}
4040
}
4141

42-
async upload(req: FastifySpaceRequest, res: FastifyReply): Promise<void> {
42+
async upload(req: FastifySpaceRequest): Promise<void> {
4343
try {
4444
await this.filesManager.saveMultipart(req.user, req.space, req)
4545
} catch (e) {
46-
this.logger.error(`${this.upload.name} - unable to ${FILE_OPERATION.UPLOAD} ${req.space.url} : ${e}`)
47-
return res
48-
.header('Connection', 'close')
49-
.status(e.httpCode || 500)
50-
.send({ message: e.message })
46+
this.handleError(req.space, FILE_OPERATION.UPLOAD, e)
5147
}
5248
}
5349

@@ -174,6 +170,7 @@ export class FilesMethods {
174170

175171
private handleError(space: SpaceEnv, action: string, e: any, dstSpace?: SpaceEnv) {
176172
this.logger.error(`unable to ${action} ${space.url}${dstSpace?.url ? ` -> ${dstSpace.url}` : ''} : ${e}`)
173+
// Remove the last part to avoid exposing the path
177174
const errorMsg = e.message.split(',')[0]
178175
if (e instanceof LockConflict) {
179176
throw new HttpException('The file is locked', HttpStatus.LOCKED)

‎backend/src/applications/sync/services/sync-manager.service.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,7 @@ export class SyncManager {
266266

267267
private handleError(space: SpaceEnv, action: string, e: any, dstSpace?: SpaceEnv) {
268268
this.logger.error(`unable to ${action} ${space.url}${dstSpace?.url ? ` -> ${dstSpace.url}` : ''} : ${e}`)
269+
// Remove the last part to avoid exposing the path
269270
const errorMsg = e.message.split(',')[0]
270271
if (e instanceof LockConflict) {
271272
throw new HttpException('The file is locked', HttpStatus.LOCKED)

‎backend/src/applications/webdav/services/webdav-methods.service.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,6 +463,7 @@ export class WebDAVMethods {
463463

464464
private handleError(req: FastifyDAVRequest, res: FastifyReply, e: any, toUrl?: string) {
465465
this.logger.error(`Unable to ${req.method} ${req.dav.url}${toUrl ? ` -> ${toUrl}` : ''} : ${e.message}`)
466+
// Remove the last part to avoid exposing the path
466467
const errorMsg = e.message.split(',')[0]
467468
if (e instanceof LockConflict) {
468469
return DAV_ERROR_RES(HttpStatus.LOCKED, PRECONDITION.LOCK_CONFLICT, res, e.lock.davLock?.lockroot || e.lock.dbFilePath)

0 commit comments

Comments
 (0)