fix: API rename and move_files allowed overwriting an existing file for which you don't have delete permission (only permission on the source was performed)
Massimo Melina committed
Jan 17, 2026 at 15:31 UTC
523e8f4a72f11b886e1f1e4bd19ea9b309493b49
3 files changed
+79
-22
src/frontEndApis.ts
+11
-4
@@ -5,7 +5,7 @@ import { get_file_list } from './api.get_file_list'
5
import * as api_auth from './api.auth'
6
import events from './events'
7
import Koa from 'koa'
8
-import { hasDirTraversal, isValidFileName } from './util-files'
8
+import { isValidFileName } from './util-files'
9
import {
10
HTTP_BAD_REQUEST, HTTP_CONFLICT, HTTP_FAILED_DEPENDENCY, HTTP_FORBIDDEN, HTTP_METHOD_NOT_ALLOWED,
11
HTTP_NOT_FOUND, HTTP_SERVER_ERROR, HTTP_UNAUTHORIZED
@@ -90,9 +90,12 @@ export const frontEndApis: ApiHandlers = {
90
throw new ApiError(HTTP_FORBIDDEN)
91
if (statusCodeForMissingPerm(node, 'can_delete', ctx))
92
throw new ApiError(ctx.status)
93
+ if (!node.source)
94
+ throw new ApiError(HTTP_FAILED_DEPENDENCY)
95
+ const destNode = await urlToNode(dest, ctx, node.parent)
96
+ if (destNode && statusCodeForMissingPerm(destNode, 'can_delete', ctx)) // if destination exists, you need delete permission
97
+ throw new ApiError(ctx.status)
98
try {
94
- if (!node.source)
95
- throw new ApiError(HTTP_FAILED_DEPENDENCY)
99
const destSource = join(dirname(node.source), dest)
100
await rename(node.source, destSource)
101
getCommentFor(node.source).then(c => {
@@ -120,7 +123,11 @@ export const frontEndApis: ApiHandlers = {
123
const srcNode = await urlToNode(from1, ctx)
124
const src = srcNode?.source
125
if (!src) return HTTP_NOT_FOUND
123
- const dest = join(destNode!.source!, basename(src))
126
+ const destName = basename(src)
127
+ const destChild = await urlToNode(destName, ctx, destNode!)
128
+ if (destChild && statusCodeForMissingPerm(destChild, 'can_delete', ctx))
129
+ return ctx.status
130
+ const dest = join(destNode!.source!, destName)
131
if (_.isFunction(override))
132
return override?.(srcNode, dest)
133
return statusCodeForMissingPerm(srcNode, 'can_delete', ctx)
tests/config.yaml
+6
-1
@@ -4,7 +4,7 @@ allowed_referer: x.com
4
localhost_admin: false
5
vfs:
6
masks:
7
- "**/config.yaml*|test.ts|work|*uploaded|work2":
7
+ "**/config.yaml*|test.ts|work|*uploaded|work2|tmp":
8
can_see: false
9
tests/page/!*.png:
10
mime: text/plain
@@ -50,6 +50,11 @@ vfs:
50
- name: no-upload
51
source: ../tmp/no-upload
52
can_upload: false
53
+ - name: cant-overwrite
54
+ source: ../tmp/cant-overwrite
55
+ can_upload:
56
+ - admins
57
+ can_delete: false
58
- source: ../alfa.txt
59
- name: for-disabled
60
can_list:
tests/test.ts
+62
-17
@@ -27,6 +27,8 @@ const BASE_URL = 'http://[::1]:81'
27
const BASE_URL_127 = 'http://127.0.0.1:81'
28
const UPLOAD_ROOT = '/for-admins/upload/'
29
const UPLOAD_DIR = 'temp'
30
+const CANT_OVERWRITE_NAME = 'cant-overwrite'
31
+const CANT_OVERWRITE_URI = `/for-admins/${CANT_OVERWRITE_NAME}/`
32
const UPLOAD_RELATIVE = `${UPLOAD_DIR}/gpl.png`
33
const UPLOAD_DEST = UPLOAD_ROOT + UPLOAD_RELATIVE
34
const BIG_CONTENT = _.repeat(randomId(10), 300_000) // 3MB, big enough to saturate buffers
@@ -35,20 +37,6 @@ const SAMPLE_FILE_PATH = resolve(__dirname, 'page/gpl.png')
37
let defaultBaseUrl = BASE_URL
38
const execP = (cmd: string) => promisify(exec)(cmd).then(x => x.stdout)
39
38
-class StringRepeaterStream extends Readable {
39
- constructor(private str: string, private n: number, readonly length=n*str.length) {
40
- super()
41
- }
42
- _read() {
43
- this.push(this.n-- > 0 ? this.str : null)
44
- }
45
-}
46
-
47
-function makeReadableThatTakes(ms: number) {
48
- return Object.assign(Readable.from(BIG_CONTENT).pipe(new ThrottledStream(new ThrottleGroup(BIG_CONTENT.length / ms))),
49
- { length: BIG_CONTENT.length })
50
-}
51
-
40
describe('basics', () => {
41
//before(async () => appStarted)
42
test('frontend', req('/', /<body>/, { headers: { accept: '*/*' } })) // workaround: 'accept' is necessary when running server-for-test-dev, still don't know why
@@ -247,6 +235,19 @@ describe('after-login', () => {
235
test('create_folder', reqApi('create_folder', { uri: UPLOAD_ROOT, name: 'temp' }, 200))
236
test('inherit.perm', reqList('/for-admins/', { inList:['alfa.txt'] }))
237
test('inherit.disabled', reqList('/for-disabled/', 401))
238
+ test('rename.to existing folder', async () => {
239
+ const from = 'rename'
240
+ const dest = 'cant-overwrite'
241
+ const baseDir = await ensureCantOverwriteDir()
242
+ const fromPath = resolve(baseDir, from)
243
+ const destPath = resolve(baseDir, dest)
244
+ await writeFile(fromPath, 'from')
245
+ await writeFile(destPath, 'dest')
246
+ try { await reqApi('rename', { uri: CANT_OVERWRITE_URI + from, dest }, 403)() }
247
+ finally {
248
+ await rmAny(baseDir)
249
+ }
250
+ })
251
test('upload.never', reqUpload('/random', 403))
252
test('upload.ok', reqUpload(UPLOAD_DEST, 200))
253
test('upload.dot name', reqUpload(`${UPLOAD_ROOT}%2e`, 418))
@@ -275,12 +276,36 @@ describe('after-login', () => {
276
await rmAny(resolve(ROOT, name))
277
}
278
})
279
+ test('move.overwrite needs delete', async () => {
280
+ const destFile = 'locked.txt'
281
+ const destDir = await ensureCantOverwriteDir()
282
+ const destPath = resolve(destDir, destFile)
283
+ const sourceUri = `${UPLOAD_ROOT}${UPLOAD_DIR}/${destFile}`
284
+ await writeFile(destPath, 'dest')
285
+ try {
286
+ await reqUpload(sourceUri, 200, 'source')()
287
+ const before = statSync(destPath).size
288
+ await reqApi('move_files', { uri_from: [sourceUri], uri_to: CANT_OVERWRITE_URI }, res => res?.errors?.[0] === 403)()
289
+ const after = statSync(destPath).size
290
+ if (after !== before)
291
+ throw "file overwritten"
292
+ }
293
+ finally {
294
+ await rmAny(resolve(ROOT, UPLOAD_DIR, destFile))
295
+ await rmAny(destDir)
296
+ }
297
+ })
298
test('upload.path bypass', async () => {
299
const name = 'no-upload'
300
const targetDir = resolve(ROOT, 'tmp', name)
281
- await execP(`curl -g -s -u ${auth} -F "upload=@${SAMPLE_FILE_PATH};filename=${name}/evil.txt" ${BASE_URL}${UPLOAD_ROOT}`)
282
- if (existsSync(resolve(targetDir, 'evil.txt')))
283
- throw "file created"
301
+ try {
302
+ await execP(`curl -g -s -u ${auth} -F "upload=@${SAMPLE_FILE_PATH};filename=${name}/evil.txt" ${BASE_URL}${UPLOAD_ROOT}`)
303
+ if (existsSync(resolve(targetDir, 'evil.txt')))
304
+ throw "file created"
305
+ }
306
+ finally {
307
+ await rmAny(targetDir)
308
+ }
309
})
310
test('upload.existing.skip', async () => {
311
const filePath = resolve(__dirname, UPLOAD_RELATIVE)
@@ -555,3 +580,23 @@ function throwIf(msg: any) {
580
if (msg)
581
throw msg
582
}
583
+
584
+async function ensureCantOverwriteDir() {
585
+ const baseDir = resolve(ROOT, 'tmp', CANT_OVERWRITE_NAME)
586
+ await mkdir(baseDir, { recursive: true })
587
+ return baseDir
588
+}
589
+
590
+class StringRepeaterStream extends Readable {
591
+ constructor(private str: string, private n: number, readonly length=n*str.length) {
592
+ super()
593
+ }
594
+ _read() {
595
+ this.push(this.n-- > 0 ? this.str : null)
596
+ }
597
+}
598
+
599
+function makeReadableThatTakes(ms: number) {
600
+ return Object.assign(Readable.from(BIG_CONTENT).pipe(new ThrottledStream(new ThrottleGroup(BIG_CONTENT.length / ms))),
601
+ { length: BIG_CONTENT.length })
602
+}