consider illegal URI encoding as not-found
Massimo Melina committed
Jan 17, 2026 at 19:49 UTC
13d54ffb65361b977c8bdb0ede1bd33a35c8159e
7 files changed
+36
-38
admin/src/LogsPage.ts
+1
-1
@@ -317,7 +317,7 @@ export function LogFile({ file, footerSide, hidden, limit, filter, ...rest }: Lo
317
sx: { wordBreak: 'break-all' }, // be flexible, uri can be a mess
318
mergeRender: { method: {}, status: {} },
319
renderCell: ({ value, row }) => {
320
- const [path, query] = splitAt('?', value).map(safeDecodeURIComponent)
320
+ const [path, query] = splitAt('?', value).map(x => safeDecodeURIComponent(x))
321
const ul = row.extra?.ul
322
if (_.isArray(ul))
323
return path + ul.join(' + ')
src/cross.ts
+4
-6
@@ -197,11 +197,9 @@ export function setHidden<T, ADD>(dest: T, src: ADD) {
197
}
198
199
export function try_<T,E=undefined>(cb: () => T, onException?: (e:any) => E) {
200
- try {
201
- return cb()
202
- }
200
+ try { return cb() }
201
catch(e) {
204
- return onException?.(e)
202
+ return onException?.(e) as E
203
}
204
}
205
@@ -525,9 +523,9 @@ export function callable<T>(x: Functionable<T>, ...args: unknown[]) {
523
return _.isFunction(x) ? x(...args) : x
524
}
525
528
-export function safeDecodeURIComponent(s: string) {
526
+export function safeDecodeURIComponent(s: string, fallback: string=s) {
527
try { return decodeURIComponent(s) }
530
- catch { return s }
528
+ catch { return fallback }
529
}
530
531
export function popKey(o: any, k: string) {
src/middlewares.ts
+9
-16
@@ -59,24 +59,17 @@ export const someSecurity: Koa.Middleware = (ctx, next) => {
59
ss.ip = ctx.ip
60
}
61
62
- try {
63
- if (hasDirTraversal(decodeURI(ctx.path)))
64
- return ctx.status = HTTP_FOOL
65
- if (!ctx.state.skipFilters && applyBlock(ctx.socket, ctx.ip))
66
- return
62
+ if (!ctx.state.skipFilters && applyBlock(ctx.socket, ctx.ip))
63
+ return
64
68
- if (ctx.get('X-Forwarded-For')
69
- // we have some dev-proxies to ignore
70
- && !(DEV && [process.env.FRONTEND_PROXY, process.env.ADMIN_PROXY].includes(ctx.get('X-Forwarded-port')))) {
71
- proxyDetected = ctx
72
- ctx.state.whenProxyDetected = new Date()
73
- }
74
- if (ctx.get('cf-ray'))
75
- cloudflareDetected = new Date()
76
- }
77
- catch {
78
- return ctx.status = HTTP_FOOL
65
+ if (ctx.get('X-Forwarded-For')
66
+ // we have some dev-proxies to ignore
67
+ && !(DEV && [process.env.FRONTEND_PROXY, process.env.ADMIN_PROXY].includes(ctx.get('X-Forwarded-port')))) {
68
+ proxyDetected = ctx
69
+ ctx.state.whenProxyDetected = new Date()
70
}
71
+ if (ctx.get('cf-ray'))
72
+ cloudflareDetected = new Date()
73
if (!ctx.secure && forceHttps.get() && getHttpsWorkingPort() && !isLocalHost(ctx)) {
74
const { URL } = ctx
75
URL.protocol = 'https'
src/serveGuiAndSharedFiles.ts
+5
-5
@@ -18,7 +18,7 @@ import { preventAdminAccess, favicon } from './adminApis'
18
import { serveGuiFiles } from './serveGuiFiles'
19
import mount from 'koa-mount'
20
import { baseUrl } from './listen'
21
-import { asyncGeneratorToReadable, filterMapGenerator, loadFileCached, pathEncode, try_ } from './misc'
21
+import { asyncGeneratorToReadable, filterMapGenerator, isValidFileName, loadFileCached, pathEncode, try_ } from './misc'
22
import XXH from 'xxhashjs'
23
import fs from 'fs'
24
import { rm } from 'fs/promises'
@@ -60,17 +60,17 @@ export const serveGuiAndSharedFiles: Koa.Middleware = async (ctx, next) => {
60
const getUploadTempHash = get === UPLOAD_TEMP_HASH
61
if (ctx.method === 'PUT' || getUploadTempHash) { // PUT is what you get with `curl -T file url/`
62
const decPath = decodeURIComponent(path)
63
- const rest = basename(decPath)
63
+ const fn = basename(decPath)
64
const folderUri = pathEncode(dirname(decPath)) // re-encode to get readable urls
65
const folder = await urlToNode(folderUri, ctx, vfs, true)
66
if (!folder)
67
return sendErrorPage(ctx, HTTP_NOT_FOUND)
68
ctx.state.uploadPath = decPath
69
if (getUploadTempHash)
70
- return !folder.source ? sendErrorPage(ctx, HTTP_NOT_FOUND)
70
+ return !folder.source || !isValidFileName(fn) ? sendErrorPage(ctx, HTTP_NOT_FOUND)
71
: statusCodeForMissingPerm(folder, 'can_upload', ctx) ? null
72
- : ctx.body = await loadFileCached(getUploadTempFor(join(folder.source, rest)), calcHash) // negligible memory leak
73
- const dest = uploadWriter(folder, folderUri, rest, ctx)
72
+ : ctx.body = await loadFileCached(getUploadTempFor(join(folder.source, fn)), calcHash) // negligible memory leak
73
+ const dest = uploadWriter(folder, folderUri, fn, ctx)
74
if (dest) {
75
ctx.req.pipe(dest).on('error', err => {
76
ctx.status = HTTP_SERVER_ERROR
src/vfs.ts
+5
-2
@@ -114,9 +114,12 @@ export async function urlToNode(
114
while (url[initialSlashes] === '/')
115
initialSlashes++
116
let nextSlash = url.indexOf('/', initialSlashes)
117
- const name = decodeURIComponent(url.slice(initialSlashes, nextSlash < 0 ? undefined : nextSlash))
118
- if (!name)
117
+ const slice = url.slice(initialSlashes, nextSlash < 0 ? undefined : nextSlash)
118
+ if (!slice)
119
return parent
120
+ const name = try_(() => decodeURIComponent(slice))
121
+ if (!name) // failed decoding
122
+ return
123
const hasTrailingSlash = url.endsWith('/')
124
const rest = nextSlash < 0 ? '' : url.slice(nextSlash+1, hasTrailingSlash ? -1 : undefined)
125
const allowMissing = resolveMissing === true
src/zip.ts
+2
-2
@@ -19,7 +19,7 @@ export async function zipStreamFromFolder(node: VfsNode, ctx: Koa.Context) {
19
ctx.status = HTTP_OK
20
ctx.mime = 'zip'
21
// ctx.query.list is undefined | string | string[]
22
- const name = list?.length === 1 ? safeDecodeURIComponent(basename(list[0]!)) : getNodeName(node)
22
+ const name = list?.length === 1 ? safeDecodeURIComponent(basename(list[0]!), '') : getNodeName(node)
23
forceDownload(ctx, (isWindowsDrive(name) ? name[0] : (name || 'archive')) + '.zip')
24
const { filterName, filterComment } = paramsToFilter(ctx.query)
25
const walker = !list ? walkNode(node, { ctx, requiredPerm: 'can_archive' })
@@ -36,7 +36,7 @@ export async function zipStreamFromFolder(node: VfsNode, ctx: Koa.Context) {
36
}
37
continue
38
}
39
- let folder = dirname(safeDecodeURIComponent(uri)) // decodeURI() won't account for %23=#
39
+ let folder = dirname(decodeURIComponent(uri)) // decodeURI() won't account for %23=# . uri is safe because otherwise urlToNode above would have already returned undefined
40
folder = folder === '.' ? '' : folder + '/'
41
yield { ...subNode, name: folder + getNodeName(subNode) } // reflect relative path in archive, otherwise way may have name-clashes
42
}
tests/test.ts
+10
-6
@@ -67,13 +67,14 @@ describe('basics', () => {
67
}))
68
test('roots', req('/f2/alfa.txt', 200, { baseUrl: BASE_URL_127 })) // host 127.0.0.1 is rooted in /f1
69
test('website', req('/f1/page/', { re:/This is a test/, mime:'text/html' }))
70
- test('traversal', req('/f1/page/.%2e/.%2e/README.md', 418))
70
+ test('traversal', req('/f1/page/.%2e/.%2e/README.md', 404))
71
test('traversal.double-encoded', req('/f1/page/%252e%252e/%252e%252e/README.md', 404))
72
test('traversal.encoded-slash', req('/f1/page/%2e%2e%2f%2e%2e%2fREADME.md', 404))
73
- test('traversal.backslash', req('/f1/page/..%5c..%5cREADME.md', 418))
74
- test('traversal.to-admin', req('/f1/page/%2e%2e/%2e%2e/for-admins/alfa.txt', 418))
75
- test('traversal.mixed-dots', req('/f1/page/.%2e/%2e./README.md', 418))
76
- test('traversal.overlong-utf8', req('/f1/page/%c0%ae%c0%ae/%c0%ae%c0%ae/README.md', 418))
73
+ test('traversal.backslash', req('/f1/page/..%5c..%5cREADME.md', 404))
74
+ test('traversal.to-admin', req('/f1/page/%2e%2e/%2e%2e/for-admins/alfa.txt', 404))
75
+ test('traversal.mixed-dots', req('/f1/page/.%2e/%2e./README.md', 404))
76
+ test('traversal.overlong-utf8', req('/f1/page/%c0%ae%c0%ae/%c0%ae%c0%ae/README.md', 404))
77
+ test('bad url encoding', req('/f1/%E0%A4%A', 404))
78
test('custom mime from above', req('/tests/page/index.html', { status: 200, mime:'text/plain' }))
79
test('name encoding', req('/x%25%23x', 200))
80
@@ -89,6 +90,7 @@ describe('basics', () => {
90
test('file_details.for-admins', reqApi('get_file_details', { uris: ['/for-admins/alfa.txt'] }, res => res?.details?.[0] === false))
91
test('file_details.traversal', reqApi('get_file_details', { uris: ['/f1/%2e%2e/for-admins/alfa.txt'] }, res => res?.details?.[0] === false))
92
test('file_list.traversal', reqApi('get_file_list', { uri: '/f1/%2e%2e/for-admins' }, 404))
93
+ test('file_list.bad encoding', reqApi('get_file_list', { uri: '/f1/%E0%A4%A' }, 404))
94
test('forbidden list', req('/cantListPage/page/', 403))
95
test('forbidden list.api', reqList('/cantListPage/page/', 403))
96
test('forbidden list.admin flag', reqApi('get_file_list', { uri: '/for-admins/', admin: true }, 401))
@@ -145,6 +147,7 @@ describe('basics', () => {
147
test('zip.partial', req('/f1/?get=zip', { re:/^page$/, length: zipLength }, { headers: { Range: `bytes=${zipOfs}-${zipOfs+zipLength-1}` } }) )
148
test('zip.partial.resume', req('/f1/?get=zip', { re:/^page/, length:zipSize-zipOfs }, { headers: { Range: `bytes=${zipOfs}-` } }) )
149
test('zip.partial.end', req('/f1/f2/?get=zip', { re:/^6/, length:10 }, { headers: { Range: 'bytes=-10' } }) )
150
+ test('zip.list.bad encoding', req('/f1/?get=zip&list=%E0%A4%A', { status: 200, length: 22 })) // basically empty
151
test('zip.alfa is forbidden', req('/protectFromAbove/child/?get=zip&list=alfa.txt//renamed', { empty: true, length:134 }, { method:'HEAD' }))
152
test('zip.cantReadPage', req('/cantReadPage/?get=zip', { length: 4832 }, { method:'HEAD' }))
153
@@ -251,6 +254,7 @@ describe('after-login', () => {
254
test('upload.never', reqUpload('/random', 403))
255
test('upload.ok', reqUpload(UPLOAD_DEST, 200))
256
test('upload.dot name', reqUpload(`${UPLOAD_ROOT}%2e`, 418))
257
+ test('upload.temp hash traversal', req(`${UPLOAD_ROOT}%2e%2e?get=${UPLOAD_TEMP_HASH}`, 404))
258
test('upload.temp hash requires auth', async () => {
259
const rel = `${UPLOAD_DIR}/partial.png`
260
await reqUpload(`${UPLOAD_ROOT}${rel}?partial=1`, 204)()
@@ -315,7 +319,7 @@ describe('after-login', () => {
319
if (after !== before)
320
throw "size changed"
321
})
318
- test('upload.crossing', reqUpload(UPLOAD_DEST.replace('temp', '../..'), 418))
322
+ test('upload.crossing', reqUpload(UPLOAD_DEST.replace('temp', '../..'), 404))
323
test('upload.overlap', async () => {
324
const ms = 300
325
const first = reqUpload(UPLOAD_DEST, 200, makeReadableThatTakes(ms))()