Skip to content

Commit 4e490b7

Browse files
BeSovereignclaude
andauthored
fix(#379): Security (low): names containing U+FFFD are created by every write path but hidd
Home paths now refuse U+FFFD with PATH_INVALID through a new normalizeHomePath used by resolveInRoot and the whole local backend (PUT, tus, mkdir, move/copy/rename, WebDAV, transfers, archive import, share writes), and file-request visitor and uploader names drop it, so nothing can be planted that the listing, the ZIP export or the reindex hide; cloud sources keep such names addressable (adversarial review found the global regex broke cloud reads). The web's rename, new-folder, name-rules and migration name checks refuse U+FFFD through a shared hasUnsafeChar with the reworded inline message in all three locales; DECISIONS.md amended. Reviews run: adversarial (led to the home-only scope and the name-rules/migrate checks) and test adversary (no blocking findings; added on-disk and no-request assertions). Refs #379 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent c318221 commit 4e490b7

18 files changed

Lines changed: 172 additions & 42 deletions

‎DECISIONS.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ File content is served inline only for images (not SVG), video, audio, PDF and `
4242

4343
On-disk names that are not canonical (NFD-normalised, containing control characters, or invalid UTF-8) are skipped in listings. They cannot round-trip through the API's path rules, so showing them would offer entries the user cannot open.
4444

45+
Amended 2026-10-19 (#379): a name containing U+FFFD is treated as invalid UTF-8 and hidden too, so the path rules of a home (`normalizeHomePath`, used by `resolveInRoot` and the local backend) refuse U+FFFD like a control character (`PATH_INVALID` on every write path), and a file request visitor's file or uploader name loses it like a control character. Otherwise any member with write access, or a visitor, could plant files the listing, the ZIP download and the reindex never show. A file whose name contains U+FFFD and was put on the disk outside the app (a restored backup) stays unaddressable, as other non-canonical names do. Cloud sources keep U+FFFD: their listings show such names, so they must stay addressable there; a transfer of one into a home fails that item with `PATH_INVALID`, as a control character does. The web's name dialogs refuse U+FFFD everywhere, a simpler rule than one per source.
46+
4547
## 2026-10-14: upload content-type gate before body parsing
4648

4749
The upload route checks the content type (`application/octet-stream`) in `onRequest`, before any body parser runs. Otherwise a JSON or text content type would make Fastify buffer the whole body in memory, which is a denial of service.

‎e2e/file-actions.spec.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,24 @@ test('renaming onto a taken name says so in the open dialog; Esc keeps both file
9595
expect(await download(page, 'b.txt')).toBe('B');
9696
});
9797

98+
test('a rename to a name with U+FFFD is refused in the dialog and never sent; the list keeps the old name (#379)', async ({ page }, info) => {
99+
const dir = `rename-fffd-${tag(info)}`;
100+
await setup(page, dir, { 'a.txt': 'A' });
101+
const moves: string[] = [];
102+
page.on('request', (r) => { if (r.url().includes('/api/v1/sources/home/move')) moves.push(r.url()); });
103+
await row(page, 'a.txt').focus();
104+
await page.keyboard.press('F2');
105+
const input = renameDialog(page).getByRole('textbox');
106+
await input.fill('x�.txt');
107+
await page.keyboard.press('Enter');
108+
await expect(input).toHaveAccessibleDescription(/kein „�“|or “�”/);
109+
await expect(input).toHaveAttribute('aria-invalid', 'true');
110+
await page.keyboard.press('Escape');
111+
await expect(renameDialog(page)).toHaveCount(0);
112+
expect(await names(page)).toEqual(['a.txt']);
113+
expect(moves).toEqual([]);
114+
});
115+
98116
test('drag and drop onto a folder moves the item into it', async ({ page }, info) => {
99117
test.skip(isMobile(info), 'touch has no HTML5 drag and drop; the action menu test covers moving on mobile');
100118
const dir = `dnd-${tag(info)}`;

‎e2e/new-folder.spec.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,14 @@ test('the name is checked; a taken name says so and keeps the dialog open; Esc c
7373
await input.fill('a/b');
7474
await page.keyboard.press('Enter');
7575
await expect(d).toContainText(/darf nicht leer|can’t be empty/);
76+
// #379: U+FFFD is refused here like a control character, before anything is sent.
77+
const mkdirs: string[] = [];
78+
page.on('request', (r) => { if (r.url().includes('/api/v1/sources/home/mkdir')) mkdirs.push(r.url()); });
79+
await input.fill('x�');
80+
await page.keyboard.press('Enter');
81+
await expect(d).toContainText(/kein „�“|or “�”/);
82+
await expect(input).toHaveAttribute('aria-invalid', 'true');
83+
expect(mkdirs).toEqual([]);
7684

7785
await input.fill('Ordner');
7886
await page.keyboard.press('Enter');

‎server/src/storage/fileRequests.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ const OPEN = '(u.placed_at IS NULL AND (u.upload_id IS NULL OR u.upload_id IN (S
2121
export const MAX_OPEN_PER_VISITOR = 10;
2222
export const MAX_OPEN_PER_REQUEST = 100;
2323

24-
// Control characters and bidi marks, as `normalizeUserPath` refuses them; here they are dropped instead.
25-
const UNSAFE = /[\p{Cc}\p{Bidi_Control}]/gu;
24+
// Control characters, bidi marks and U+FFFD (#379), as `normalizeHomePath` refuses them; here they are dropped instead.
25+
const UNSAFE = /[\p{Cc}\p{Bidi_Control}�]/gu;
2626
const MAX_NAME_BYTES = 255;
2727
/** The name of an upload whose name has nothing left after sanitising. */
2828
export const FALLBACK_NAME = 'upload';

‎server/src/storage/local.ts‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { pipeline } from 'node:stream/promises';
77
import type { FastifyBaseLogger } from 'fastify';
88
import type { CollisionPolicy, Entry, HomeUsage, LargeFolder, OpenUpload, ThumbSize, TrashItem } from '@files/shared';
99
import { AppError } from '../errors.ts';
10-
import { normalizeUserPath, resolveInRoot } from './resolveInRoot.ts';
10+
import { normalizeHomePath, resolveInRoot } from './resolveInRoot.ts';
1111
import { RangeNotSatisfiable, type ByteRange, type Revision, type StorageBackend, type WriteOptions } from './backend.ts';
1212
import { lookupMime } from './mime.ts';
1313
import type { Db } from '../db.ts';
@@ -109,10 +109,10 @@ function decodeCursor(cursor: string): SortKey {
109109
return { d: k.d, n: k.n };
110110
}
111111

112-
// A listed name must round-trip through normalizeUserPath unchanged, else it could not be addressed again.
112+
// A listed name must round-trip through normalizeHomePath unchanged, else it could not be addressed again.
113113
function addressable(name: string): boolean {
114114
if (name.includes('\uFFFD') || TEMP_NAME.test(name)) return false; // U+FFFD: invalid UTF-8 on disk
115-
try { return normalizeUserPath('/' + name) === '/' + name; }
115+
try { return normalizeHomePath('/' + name) === '/' + name; }
116116
catch { return false; } // handled: unaddressable on-disk name is skipped
117117
}
118118

@@ -214,15 +214,15 @@ export class LocalBackend implements StorageBackend {
214214

215215
// Every user path goes through here: normalise, refuse the internal area and temp names, resolve inside the root.
216216
private async abs(p: string) {
217-
const n = normalizeUserPath(p);
217+
const n = normalizeHomePath(p);
218218
if (n === `/${INTERNAL}` || n.startsWith(`/${INTERNAL}/`)) throw new AppError('PATH_INVALID');
219219
if (n !== '/' && TEMP_NAME.test(basename(n))) throw new AppError('PATH_INVALID');
220220
await mkdir(this.root, { recursive: true });
221221
return resolveInRoot(this.root, n);
222222
}
223223

224224
private entry(userPath: string, st: { isDirectory(): boolean; size: number; mtime: Date }): Entry {
225-
const path = normalizeUserPath(userPath);
225+
const path = normalizeHomePath(userPath);
226226
const name = path === '/' ? '/' : basename(path);
227227
const isDir = st.isDirectory();
228228
return { name, path, type: isDir ? 'directory' : 'file', size: isDir ? 0 : st.size,
@@ -255,14 +255,14 @@ export class LocalBackend implements StorageBackend {
255255

256256
/** The kept versions of a file (#154), oldest first; the file itself is not one of them. */
257257
async revisions(p: string): Promise<Revision[]> {
258-
const n = normalizeUserPath(p);
258+
const n = normalizeHomePath(p);
259259
await this.statFile(n);
260260
return this.liveVersions(n).map((v) => ({ id: v.id, modified: v.modified, size: v.size }));
261261
}
262262

263263
/** The bytes of one kept version of the file at `p`; a version of any other file is NOT_FOUND. */
264264
async readRevision(p: string, id: string): Promise<Awaited<ReturnType<StorageBackend['read']>>> {
265-
const n = normalizeUserPath(p);
265+
const n = normalizeHomePath(p);
266266
await this.abs(n);
267267
const row = this.versionRow(n, id);
268268
if (!row) throw new AppError('NOT_FOUND');
@@ -288,7 +288,7 @@ export class LocalBackend implements StorageBackend {
288288
restoreRevision(p: string, id: string): Promise<Entry> { return this.restoreRevisionImpl(p, id).catch(rethrowMapped); }
289289

290290
private async restoreRevisionImpl(p: string, id: string): Promise<Entry> {
291-
const n = normalizeUserPath(p);
291+
const n = normalizeHomePath(p);
292292
const target = await this.abs(n);
293293
if (!this.versionRow(n, id)) throw new AppError('NOT_FOUND');
294294
await this.statFile(n);
@@ -365,7 +365,7 @@ export class LocalBackend implements StorageBackend {
365365
const st = await lstat(a).then((s) => s, (e) => { if (isErr(e, 'ENOENT')) throw new AppError('NOT_FOUND'); throw e; });
366366
if (st.isDirectory()) throw new AppError('IS_A_DIRECTORY');
367367
if (!st.isFile()) throw new AppError('NOT_FOUND');
368-
return thumbs.get({ userId, path: normalizeUserPath(p), abs: a, etag: etagOf(this.index.etagKey, st), mime: lookupMime(basename(a)) }, size);
368+
return thumbs.get({ userId, path: normalizeHomePath(p), abs: a, etag: etagOf(this.index.etagKey, st), mime: lookupMime(basename(a)) }, size);
369369
}
370370

371371
// After a write, move or delete has landed: the thumbnails of the paths it touched (and beneath them) go. A stale one
@@ -418,7 +418,7 @@ export class LocalBackend implements StorageBackend {
418418
}
419419

420420
private async endpoints(from: string, to: string, policy: CollisionPolicy) {
421-
const nf = normalizeUserPath(from); const nt = normalizeUserPath(to);
421+
const nf = normalizeHomePath(from); const nt = normalizeHomePath(to);
422422
if (nf === '/' || nt === '/') throw new AppError('PATH_INVALID');
423423
const src = await this.abs(nf); const dst = await this.abs(nt);
424424
const st = await lstat(src).then((s) => s, (e) => { if (isErr(e, 'ENOENT')) throw new AppError('NOT_FOUND'); throw e; });
@@ -521,7 +521,7 @@ export class LocalBackend implements StorageBackend {
521521

522522
private async deleteImpl(paths: readonly string[]): Promise<void> {
523523
if (paths.length === 0) throw new AppError('VALIDATION');
524-
const norm = [...new Set(paths.map(normalizeUserPath))];
524+
const norm = [...new Set(paths.map(normalizeHomePath))];
525525
if (norm.includes('/')) throw new AppError('PATH_INVALID');
526526
// An item beneath another selected one goes to the trash with it.
527527
const top = norm.filter((p) => !norm.some((q) => p.startsWith(q + '/')));
@@ -583,7 +583,7 @@ export class LocalBackend implements StorageBackend {
583583
if (this.index.disk && row.size > await this.index.disk.left()) throw new AppError('STORAGE_FULL');
584584
const src = await this.trashBytes(id);
585585
const trash = await this.internalDir('trash');
586-
const nt = normalizeUserPath(row.original_path);
586+
const nt = normalizeHomePath(row.original_path);
587587
const dst = await this.abs(nt); // refuses symlinks and files on the way, like any user path
588588
await mkdir(dirname(dst), { recursive: true }).catch((e) => {
589589
if (isErr(e, 'ENOTDIR') || isErr(e, 'EEXIST')) throw new AppError('NOT_A_DIRECTORY', undefined, { cause: e });
@@ -775,7 +775,7 @@ export class LocalBackend implements StorageBackend {
775775

776776
private async createUploadImpl(u: CreateUpload) {
777777
if (u.length > u.maxBytes) throw new AppError('PAYLOAD_TOO_LARGE');
778-
const nt = normalizeUserPath(u.path);
778+
const nt = normalizeHomePath(u.path);
779779
if (nt === '/') throw new AppError('PATH_INVALID');
780780
// An upload for another source (a file request into a cloud folder) is only staged here: its path is not in this home.
781781
if (u.source === 'home') {
@@ -957,7 +957,7 @@ export class LocalBackend implements StorageBackend {
957957
private async placeUpload(row: UploadRow, policy: CollisionPolicy, onPlaced?: (name: string) => void): Promise<Entry> {
958958
// Staged for another source: only its caller's `UploadPlacement` places it (a file request into a cloud folder).
959959
if (row.source !== 'home' && !onPlaced) throw new AppError('NOT_SUPPORTED');
960-
const nt = normalizeUserPath(row.path);
960+
const nt = normalizeHomePath(row.path);
961961
const target = await this.abs(nt);
962962
await mkdir(dirname(target), { recursive: true }).catch((e) => {
963963
if (isErr(e, 'ENOTDIR') || isErr(e, 'EEXIST')) throw new AppError('NOT_A_DIRECTORY', undefined, { cause: e });
@@ -1003,7 +1003,7 @@ export class LocalBackend implements StorageBackend {
10031003
if (isErr(e, 'ENOTDIR')) throw new AppError('NOT_A_DIRECTORY');
10041004
throw e;
10051005
}
1006-
const base = normalizeUserPath(p);
1006+
const base = normalizeHomePath(p);
10071007
const keyed = dirents
10081008
.filter((d) => !(base === '/' && d.name === INTERNAL) && (d.isFile() || d.isDirectory()) && addressable(d.name))
10091009
.map((d) => ({ d, key: { d: d.isDirectory() ? 1 : 0, n: d.name } as SortKey }))
@@ -1053,7 +1053,7 @@ export class LocalBackend implements StorageBackend {
10531053
const a = await this.abs(p);
10541054
await mkdir(dirname(a), { recursive: true });
10551055
await mkdir(a).catch((e) => { if (isErr(e, 'EEXIST')) throw new AppError('FILE_EXISTS'); throw e; });
1056-
this.index.db.transaction(() => this.indexPlaced(normalizeUserPath(p), a)).immediate();
1056+
this.index.db.transaction(() => this.indexPlaced(normalizeHomePath(p), a)).immediate();
10571057
return this.statImpl(p);
10581058
}
10591059

@@ -1113,7 +1113,7 @@ export class LocalBackend implements StorageBackend {
11131113
let rel: string;
11141114
if (opts.onCollision === 'replace') {
11151115
// The content being replaced is kept as a version, in the transaction that puts the new bytes in place.
1116-
rel = normalizeUserPath(p);
1116+
rel = normalizeHomePath(p);
11171117
const versions = await this.internalDir('versions');
11181118
this.atomically((fs) => {
11191119
this.keepVersion(fs, versions, target, rel);
@@ -1127,7 +1127,7 @@ export class LocalBackend implements StorageBackend {
11271127
} else {
11281128
const finalPath = await this.place(tmp, target, opts.onCollision);
11291129
await syncDir(dir);
1130-
rel = join(dirname(normalizeUserPath(p)), basename(finalPath));
1130+
rel = join(dirname(normalizeHomePath(p)), basename(finalPath));
11311131
db.transaction(() => { recordRecent(db, userId, rel, 'file'); this.indexPlaced(rel, finalPath); }).immediate();
11321132
}
11331133
await this.forgetThumbs(rel);

‎server/src/storage/resolveInRoot.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,18 @@ export function normalizeUserPath(userPath: string): string {
1818
return '/' + segments.join('/');
1919
}
2020

21+
/**
22+
* `normalizeUserPath` for a path in a home on disk, which also refuses U+FFFD (#379): a home listing hides names holding
23+
* it (invalid UTF-8 on disk, storage/local.ts `addressable`), so no path in a home may contain it and no write creates
24+
* one. Cloud sources list such names and still address them through `normalizeUserPath`.
25+
*/
26+
export function normalizeHomePath(userPath: string): string {
27+
if (typeof userPath === 'string' && userPath.includes('�')) throw new AppError('PATH_INVALID');
28+
return normalizeUserPath(userPath);
29+
}
30+
2131
export async function resolveInRoot(root: string, userPath: string): Promise<string> {
22-
const normalized = normalizeUserPath(userPath);
32+
const normalized = normalizeHomePath(userPath);
2333
const realRoot = await realpath(root);
2434
const segments = normalized.split('/').filter(Boolean);
2535
if (Buffer.byteLength(join(realRoot, ...segments)) > MAX_PATH_BYTES) throw new AppError('PATH_INVALID');

‎server/test/fileRequests.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,17 @@ describe('the visitor', () => {
196196
expect((await files(r.id)).map((f) => f.name).sort()).toEqual(['not-there.pdf', 'secret-contract (1).pdf']);
197197
});
198198

199+
it('a visitor\'s name loses U+FFFD like a control character, so the file appears in the owner\'s listing (#379)', async () => {
200+
const dir = await folder();
201+
const r = await request(dir, { askName: true });
202+
const v = await visitor(r.token);
203+
expect((await upload(r.token, v, 'invoice�.pdf', 'pdf', { uploader: 'Test� User' })).statusCode).toBe(204);
204+
expect(namesIn(dir)).toEqual(['invoice.pdf']);
205+
const listed = await ok(t.app.inject({ url: `/api/v1/sources/home/list?path=${encodeURIComponent(dir)}`, headers: { cookie: owner.cookie } }));
206+
expect((listed.json().entries as Entry[]).map((e) => e.name)).toEqual(['invoice.pdf']);
207+
expect((await files(r.id)).map((f) => [f.name, f.uploader])).toEqual([['invoice.pdf', 'Test User']]);
208+
});
209+
199210
it('names are sanitised and never leave the target folder (#94); colliding names are all kept', async () => {
200211
const parent = await folder();
201212
await ok(op('mkdir', { path: `${parent}/inbox` }));
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { existsSync, readdirSync } from 'node:fs';
2+
import { join } from 'node:path';
3+
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
4+
import { homeRoot } from '../src/paths.ts';
5+
import { startTestApp, login, type TestApp } from './support/harness.ts';
6+
import { basic, createAppPassword } from './support/dav.ts';
7+
8+
// #379: a listing hides names containing U+FFFD (they stand for invalid UTF-8 on disk), so no write path may create one:
9+
// otherwise a file request visitor or a member with `edit` plants files the owner never sees.
10+
11+
type S = { cookie: string; csrf: string };
12+
let t: TestApp; let a: S; let pw: string; let root: string;
13+
const ORIGIN = 'https://files.example.com';
14+
const mutating = (s: S) => ({ cookie: s.cookie, 'x-csrf-token': s.csrf, origin: ORIGIN });
15+
16+
beforeAll(async () => {
17+
t = await startTestApp();
18+
a = await login(t, 'testuser');
19+
pw = await createAppPassword(t, a);
20+
root = homeRoot(t.config.dataDir, (await t.app.inject({ url: '/api/v1/me', headers: { cookie: a.cookie } })).json().id);
21+
});
22+
afterAll(() => t.close());
23+
24+
const op = (name: string, payload: object) => t.app.inject({
25+
method: 'POST', url: `/api/v1/sources/home/${name}`, headers: mutating(a), payload });
26+
const put = (path: string, text: string) => t.app.inject({
27+
method: 'PUT', url: `/api/v1/sources/home/content?path=${encodeURIComponent(path)}&onCollision=fail`,
28+
headers: { ...mutating(a), 'content-type': 'application/octet-stream' }, payload: Buffer.from(text) });
29+
const list = async (path: string) => {
30+
const r = await t.app.inject({ url: `/api/v1/sources/home/list?path=${encodeURIComponent(path)}`, headers: { cookie: a.cookie } });
31+
expect(r.statusCode, r.body).toBe(200);
32+
return (r.json().entries as { name: string }[]).map((e) => e.name).sort();
33+
};
34+
35+
describe('names containing U+FFFD are refused on every write path (#379)', () => {
36+
it('PUT, mkdir, move and copy answer PATH_INVALID and create nothing; the listing shows what exists', async () => {
37+
expect((await op('mkdir', { path: '/probe' })).statusCode).toBe(201);
38+
const p = await put('/probe/report�.txt', 'hidden');
39+
expect(p.statusCode, p.body).toBe(400);
40+
expect(p.json()).toEqual({ code: 'PATH_INVALID' });
41+
expect(existsSync(join(root, 'probe', 'report�.txt'))).toBe(false);
42+
43+
const d = await op('mkdir', { path: '/probe/dir�' });
44+
expect(d.statusCode, d.body).toBe(400);
45+
expect(d.json()).toEqual({ code: 'PATH_INVALID' });
46+
47+
expect((await put('/probe/a.txt', 'a')).statusCode).toBe(201);
48+
const m = await op('move', { from: '/probe/a.txt', to: '/probe/b�.txt', onCollision: 'fail' });
49+
expect(m.statusCode, m.body).toBe(400);
50+
expect(m.json()).toEqual({ code: 'PATH_INVALID' });
51+
const c = await op('copy', { from: '/probe/a.txt', to: '/probe/c�.txt', onCollision: 'fail' });
52+
expect(c.statusCode, c.body).toBe(400);
53+
expect(c.json()).toEqual({ code: 'PATH_INVALID' });
54+
55+
// Nothing hidden was planted: the disk holds exactly what the listing shows.
56+
expect(readdirSync(join(root, 'probe'))).toEqual(['a.txt']);
57+
expect(await list('/probe')).toEqual(['a.txt']);
58+
});
59+
60+
it('WebDAV PUT and MOVE to such a name answer 400 and create nothing', async () => {
61+
const dav = (method: string, path: string, headers: Record<string, string> = {}, payload?: string) => t.app.inject({
62+
method: method as 'PUT', url: `/dav${path}`, payload, headers: { authorization: basic(pw), ...headers } });
63+
expect((await dav('MKCOL', '/davprobe')).statusCode).toBe(201);
64+
const p = await dav('PUT', '/davprobe/%EF%BF%BD.txt', {}, 'hidden');
65+
expect(p.statusCode, p.body).toBe(400);
66+
expect(existsSync(join(root, 'davprobe', '�.txt'))).toBe(false);
67+
68+
expect((await dav('PUT', '/davprobe/a.txt', {}, 'a')).statusCode).toBe(201);
69+
const m = await dav('MOVE', '/davprobe/a.txt', { destination: `${ORIGIN}/dav/davprobe/b%EF%BF%BD.txt` });
70+
expect(m.statusCode, m.body).toBe(400);
71+
expect(readdirSync(join(root, 'davprobe'))).toEqual(['a.txt']);
72+
expect(await list('/davprobe')).toEqual(['a.txt']);
73+
});
74+
});

0 commit comments

Comments
 (0)