Skip to content

Commit 348b6b7

Browse files
FreeShard Botclaude
andcommitted
fix(#401): Security: file-request visitor places unbounded files when n
Bounded files per file request (1000 ceiling, 100 per visitor, counted placed plus open) and rate-limited visitor upload creation (60/min per request+visitor and per address); now also LocalBackend.dropUpload deletes a visitor's unplaced file_request_uploads row together with its dropped upload (SIZE_MISMATCH overrun, expiry), and migration 032 removes such orphans, so overrunning uploads can no longer grow the table without bound. Adversarial and test-adversary reviews ran; the test adversary's blocking gap (the cloud-placed row's placed_at guard) is now pinned by the cloud test, and the PATCH ceiling and German hint wording were fixed. Refs #401 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent b739414 commit 348b6b7

17 files changed

Lines changed: 391 additions & 29 deletions

‎CONCEPTS.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ Sharing (#146): a `shares` row grants one member `read` or `edit` on one item of
7272

7373
Public links (#147): a `public_links` row lets anyone with its token (16 random bytes, base64url) reach one item of the owner's own source at `/s/<token>`, proxied through the shard. The visitor's requests go to the one public route `/s/:token/*`, which checks on every request: an unknown or revoked token is `NOT_FOUND`, a deleted item or unlinked connection `SHARE_ORPHANED` (the row is in `PATH_TABLES` like `shares`, and the owner's source is reached through the same `ownerSource`, never a fallback), a passed expiry `LINK_EXPIRED`, and without the password `PASSWORD_REQUIRED`. The password is an argon2id hash; wrong attempts are limited per link and per address (429; each attempt is reserved in the same step as the check and counts while argon2 runs, so a parallel burst gets no more checks than the limit, #196), and a correct one sets a signed cookie scoped to `/s/<token>/` that names the link and the current password. Every byte-serving answer of `content` counts against `max_downloads` (NULL = no limit, 0 = none), previews and Range requests included, in one conditional `UPDATE`. The content is served through `ShareBackend` (confined beneath the item), always with `CSP: sandbox` and `nosniff`, inline only on request and only for the allowlist. An expiry the server cannot parse is refused when the link is created or changed, never taken as "no expiry". The page `/s/<token>` is the SPA (`PublicLinkView`) under the app's CSP, outside the signed-in shell, in the visitor's language; it gets everything as JSON and renders it as text. After a download or preview it asks `info` again until the count changed (#232), and mentions the limit only for a link that has one; the owner revokes a link only after a confirmation. The visitor sees only the link's own error codes and those of their request; whatever the owner's source reports about itself (a revoked grant, a rate-limiting provider, Drive's naming) is one `LINK_UNAVAILABLE` (503), with the real code logged with the link's id (#279). Local ETags are an HMAC of inode, size and mtime under a key derived from the session secret, so a grantee or visitor does not learn the inode (#279). The provider's own tags of a cloud item pass through unchanged.
7474

75-
File requests (#217): a `file_requests` row lets anyone with its token (16 random bytes) upload into one folder of the owner's home or own cloud connection at `/r/<token>`, without seeing it. The public route `/r/:token/*` only accepts: `info` (the title, the limits left and the visitor's own uploads of this session under the names they sent, never the folder, its name or contents; not even the saved keep-both name, whose ` (1)` would reveal that the folder holds the name, #305), `unlock`, and tus creation, `HEAD` and `PATCH` under `uploads`; there is nothing to list or download. Each request is checked like a public link's (unknown `NOT_FOUND`; revoked `LINK_REVOKED`, deleted folder or unlinked connection `SHARE_ORPHANED`, passed expiry `LINK_EXPIRED`, all 410; the same password limits, with one per-address limiter shared with public links), mutating requests need the app's `Origin`, and the visitor is a random id in a signed cookie scoped to `/r/<token>/` that also carries the unlock. Every visitor upload is a tus upload of the **owner's** home (`LocalBackend.createUpload`), so the owner's quota and the size limit are checked at creation and the declared length is enforced while streaming; a cloud target also needs the provider's free space, and its bytes are staged in the owner's home and written into the cloud folder once complete (`UploadPlacement`). The name is sanitised (`sanitizeUploadName`: the last segment after `/` or `\`, no control characters, no leading dots or trailing dots and spaces, at most 255 bytes) and placed with keep-both in the folder as it is at the end (it follows moves, `PATH_TABLES`). A `file_request_uploads` row per file is reserved in one write transaction against `max_files` and `max_bytes` before the tus upload opens, so parallel creations never pass a limit; a row counts while reserved, open or received, and stops counting once its upload expired or was dropped. Open uploads hold the owner's quota (but not the shard's disk reserve, #364), so they are bounded and released (#304): they expire 15 minutes after their creation or last `PATCH` (a `PATCH` still connected keeps its upload's rest on the quota); at most 10 per visitor and 100 per request (`REQUEST_UPLOADS_OPEN`, checked in the same transaction); revoking a request terminates them, the owner can discard them while keeping the request open (`pending` in the list), and the hourly upload expiry job terminates those of any revoked, orphaned or expired request. Received rows carry the saved name, the optional uploader name (kept only if the request asks for it) and `seen_at`, which clears the owner's badge ("N new").
75+
File requests (#217): a `file_requests` row lets anyone with its token (16 random bytes) upload into one folder of the owner's home or own cloud connection at `/r/<token>`, without seeing it. The public route `/r/:token/*` only accepts: `info` (the title, the limits left and the visitor's own uploads of this session under the names they sent, never the folder, its name or contents; not even the saved keep-both name, whose ` (1)` would reveal that the folder holds the name, #305), `unlock`, and tus creation, `HEAD` and `PATCH` under `uploads`; there is nothing to list or download. Each request is checked like a public link's (unknown `NOT_FOUND`; revoked `LINK_REVOKED`, deleted folder or unlinked connection `SHARE_ORPHANED`, passed expiry `LINK_EXPIRED`, all 410; the same password limits, with one per-address limiter shared with public links), mutating requests need the app's `Origin`, and the visitor is a random id in a signed cookie scoped to `/r/<token>/` that also carries the unlock. Every visitor upload is a tus upload of the **owner's** home (`LocalBackend.createUpload`), so the owner's quota and the size limit are checked at creation and the declared length is enforced while streaming; a cloud target also needs the provider's free space, and its bytes are staged in the owner's home and written into the cloud folder once complete (`UploadPlacement`). The name is sanitised (`sanitizeUploadName`: the last segment after `/` or `\`, no control characters, no leading dots or trailing dots and spaces, at most 255 bytes) and placed with keep-both in the folder as it is at the end (it follows moves, `PATH_TABLES`). A `file_request_uploads` row per file is reserved in one write transaction against `max_files` and `max_bytes` before the tus upload opens, so parallel creations never pass a limit; a row counts while reserved, open or received, and goes with its upload when that expires or is dropped before it was placed (#401). Open uploads hold the owner's quota (but not the shard's disk reserve, #364), so they are bounded and released (#304): they expire 15 minutes after their creation or last `PATCH` (a `PATCH` still connected keeps its upload's rest on the quota); at most 10 per visitor and 100 per request (`REQUEST_UPLOADS_OPEN`, checked in the same transaction). Files themselves are bounded too, since a zero-byte file passes every byte check (#401): a request takes at most `MAX_REQUEST_FILES` (1000, also when `max_files` is null; its maximum) and one visitor session at most 100 through it, placed and open together (`REQUEST_FILE_LIMIT`; `info`'s `filesLeft` is the visitor's own), and creations are limited to 60 a minute per request and visitor and 60 per address, an IPv6 one as its /64 (429 `TOO_MANY_ATTEMPTS` with `retry-after`, which the visitor's page waits out); revoking a request terminates them, the owner can discard them while keeping the request open (`pending` in the list), and the hourly upload expiry job terminates those of any revoked, orphaned or expired request. Received rows carry the saved name, the optional uploader name (kept only if the request asks for it) and `seen_at`, which clears the owner's badge ("N new").
7676

7777
Cloud connections (#148): a `connections` row holds one provider account of one member, with the refresh token sealed by AES-256-GCM (`cloud/secretBox.ts`, bound to the row's id) and a status (`ok`, `reauth`; `unavailable` when the shard no longer offers the provider, `library` while a SharePoint connection has no library yet). Providers are definitions with fixed endpoints passed to `buildApp`; production offers each one whose client credentials are configured (`configuredProviders`: Google Drive with `FILES_GOOGLE_CLIENT_*`, #149; Dropbox with `FILES_DROPBOX_CLIENT_*`, #150; OneDrive and SharePoint with `FILES_GRAPH_CLIENT_*`, #151; pCloud with `FILES_PCLOUD_CLIENT_*`, #211), tests and the E2E cloud server bring the fake provider (`server/test/support/fakeProvider.ts`). Connecting is the authorization code flow: `state` and the PKCE verifier live only in a `connect_attempts` row bound to the session, single-use, 10 minutes; the redirect URI comes from the configured public URL; a callback without a matching state stores nothing; nothing of the callback's query is logged. `OAuthClient` (`cloud/oauth.ts`, one per connection and process) keeps the access token in memory, refreshes it before expiry or after a 401 in a single flight, writes a rotated refresh token before using the new access token, marks the connection `reauth` when the provider refuses the grant (`CONNECTION_REAUTH`), and retries 429/5xx with `Retry-After` or exponential backoff plus jitter (`PROVIDER_UNAVAILABLE` after that). `conn:<id>` resolves only among the caller's own connections. Unlinking removes the row, orphans every share and public link into it and marks the member's favourites in it gone, in one transaction (`orphanSourceRefs`); they never revive when the account is connected again.
7878

‎DECISIONS.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,3 +1367,16 @@ Question: where does the storage page get the default reserve's "5 GiB" once it
13671367
Decision: `GET /api/v1/admin/disk` returns `defaultMin`, `DEFAULT_RESERVE.bytes` in bytes, and the page formats it with `formatSize` ("5.4 GB"); the locale key `storage.disk.reserveMin` is gone. The "5 %" stays a locale string, since a percentage has no unit question. `align-items: baseline` is set on `.details-facts` itself, so the details drawer's facts get it too.
13681368

13691369
Why: the figure is defined once on the server, where the reserve is computed; a copy in `@files/shared` or the locales could drift from it. A muted label above its value's baseline is wrong in every fact list, not only on the migration pages, and one rule is less than two (YAGNI).
1370+
1371+
## 2026-10-20: A file request bounds its files, not only its bytes (#401)
1372+
1373+
Question: #304 and #364 bounded what a visitor's uploads hold (open uploads, quota, disk), but a zero-byte file passes every byte check, and with "max files" left empty a visitor placed files without end: an inode, a `file_request_uploads` row, a `name_index` and a `recent` row each. What does "no limit" mean for the number of files, and what may one visitor do?
1374+
1375+
Decision (amends #304 and #364):
1376+
- `MAX_REQUEST_FILES` = 1000 (`@files/shared`) is the most files one request ever takes: the bound when `maxFiles` is null, the schema's new maximum (was 100000), and a cap over a larger value stored before. One visitor session places at most `MAX_REQUEST_FILES_PER_VISITOR` = 100 through one request. Both count placed and open files (`COUNTS`) in `reserveUpload`'s transaction and answer `REQUEST_FILE_LIMIT`.
1377+
- Visitor upload creation is limited to 60 per fixed one-minute window per (request, visitor session) and 60 per address (`rateLimit` over two `AttemptLimiter`s in `http/attempts.ts`); an IPv6 address counts as its /64 (`addressKey`), an IPv4-mapped one as its IPv4. Every attempt counts, the 61st answers 429 `TOO_MANY_ATTEMPTS` with `retry-after` and is not counted itself. The visitor's page waits out `retry-after` and tries the creation again, twice at most, so a drop of more than 60 files (or a family behind one address) only slows down; refused a third time, the file shows "Too many files in a short time", not the password wording of `errors.TOO_MANY_ATTEMPTS`.
1378+
- Migration 032 sets a stored `max_files` above 1000 to 1000, so the owner sees the limit in force and the edit dialog still saves.
1379+
- A visitor's file whose upload is dropped before it is placed (overrun with `SIZE_MISMATCH`, expired, terminated) loses its `file_request_uploads` row in the same step (`LocalBackend.dropUpload`); migration 032 deletes the ones left before. Such a row counted against nothing, so overrunning uploads on purpose would otherwise have grown the table without end below every cap.
1380+
- `info`'s `filesLeft` is the visitor's own: the smaller of what the request and the visitor have left, so a limitless request shows a fresh visitor 100. The owner keeps seeing `maxFiles` as set (null); the dialog's files field shows "Up to 1,000" as placeholder, and the hint names both caps.
1381+
1382+
Why: the per-visitor cap alone can be dodged by dropping the cookie, so the per-request ceiling is the real bound and the per-address rate slows refilling it; a visitor cannot exhaust a request for others faster than one minute per 60 files from one address. 1000 files and 100 per sender cover a family or team collecting photos or papers; more is a second request. Placed files keep counting after the owner moves or deletes them, so a limitless request that already received 1000 files is full from this change on, and senders from many addresses can fill one within minutes; the owner then creates a new request (YAGNI: no reset, no "full" badge until someone asks). Showing the visitor their own number (not the request's 1000) is what will actually stop them (YAGNI: no second figure).

‎e2e/file-requests.spec.ts‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,87 @@ test('a request with a file limit refuses the next file; a revoked request says
160160
await v.context().close();
161161
});
162162

163+
/**
164+
* #401: creates `/<dir>` and a request without limits on it through the API, and returns its address. `cleanUp`
165+
* deletes the folder and purges it from the trash again: the home root windows from 200 entries (#225), and other
166+
* specs expect their own folders to render in it.
167+
*/
168+
async function apiHeaders(page: Page) {
169+
const { csrfToken } = await (await page.request.get('/api/v1/me')).json();
170+
return { 'x-csrf-token': csrfToken, origin: new URL(page.url()).origin };
171+
}
172+
async function limitlessRequest(page: Page, dir: string, label: string): Promise<string> {
173+
const headers = await apiHeaders(page);
174+
expect((await page.request.post('/api/v1/sources/home/mkdir', { headers, data: { path: `/${dir}` } })).status()).toBe(201);
175+
const r = await page.request.post('/api/v1/requests', { headers, data: { source: 'home', path: `/${dir}`, label } });
176+
expect(r.status()).toBe(201);
177+
return `${headers.origin}/r/${(await r.json()).token}`;
178+
}
179+
async function cleanUp(page: Page, dir: string) {
180+
const headers = await apiHeaders(page);
181+
const del = await page.request.post('/api/v1/sources/home/delete', { data: { paths: [`/${dir}`] }, headers });
182+
expect(del.status(), await del.text()).toBeLessThan(300);
183+
const trash = await (await page.request.get('/api/v1/trash')).json() as { items: { id: string; originalPath: string }[] };
184+
const ids = trash.items.filter((i) => i.originalPath === `/${dir}`).map((i) => i.id);
185+
const purge = await page.request.post('/api/v1/trash/delete', { data: { ids }, headers });
186+
expect(purge.status(), await purge.text()).toBe(204);
187+
}
188+
189+
test('#401: a request without a file limit shows the ceiling to the owner and the files left to the visitor', async ({ page, browser, baseURL }, info) => {
190+
await page.goto('/');
191+
await expect(page.getByRole('navigation', { name: /Pfad|Path/ })).toBeVisible();
192+
await openRequests(page);
193+
await page.getByRole('button', { name: /Neue Dateianfrage|New file request/ }).click();
194+
const d = page.getByRole('dialog', { name: /Neue Dateianfrage|New file request/ });
195+
await expect(d).toBeVisible();
196+
const files = d.getByLabel(/Höchstens so viele Dateien|Maximum number of files/);
197+
await expect(files).toHaveValue('');
198+
await expect(files).toHaveAttribute('placeholder', /^(Bis zu 1\.000|Up to 1,000)$/);
199+
await expect(d).toContainText(/(höchstens 1\.000 Dateien an, höchstens 100 von jeder Person|at most 1,000 files, at most 100 from each sender)/);
200+
await page.keyboard.press('Escape');
201+
await expect(d).toHaveCount(0);
202+
const dir = `request-ceiling-${tag(info)}`;
203+
const address = await limitlessRequest(page, dir, `No limit ${tag(info)}`);
204+
205+
const v = await visitor(browser, info, baseURL!);
206+
await v.goto(address);
207+
await expect(v.getByTestId('files-left')).toHaveText('100');
208+
await v.getByTestId('request-picker').setInputFiles([{ name: 'empty.txt', mimeType: 'text/plain', buffer: Buffer.alloc(0) }]);
209+
await expect(v.getByRole('list', { name: 'Your files' }).getByRole('listitem').filter({ hasText: 'empty.txt' })).toContainText('Sent');
210+
await expect(v.getByTestId('files-left')).toHaveText('99');
211+
await v.context().close();
212+
await cleanUp(page, dir);
213+
});
214+
215+
test('#401: a visitor\'s page waits out the creation rate limit; refused again and again, it says why', async ({ page, browser, baseURL }, info) => {
216+
await page.goto('/');
217+
await expect(page.getByRole('navigation', { name: /Pfad|Path/ })).toBeVisible();
218+
const dir = `request-rate-${tag(info)}`;
219+
const address = await limitlessRequest(page, dir, `Rate ${tag(info)}`);
220+
const v = await visitor(browser, info, baseURL!);
221+
// The server's answer to a creation over the rate, as `rateLimit` sends it; `refuse` says how many creations get it.
222+
let refuse = 1; const refusedAt: number[] = [];
223+
await v.route(/\/r\/[^/]+\/uploads$/, async (route) => {
224+
if (route.request().method() !== 'POST' || refuse === 0) return route.fallback();
225+
refuse--; refusedAt.push(Date.now());
226+
return route.fulfill({ status: 429, headers: { 'retry-after': '2', 'content-type': 'application/json' }, body: JSON.stringify({ code: 'TOO_MANY_ATTEMPTS' }) });
227+
});
228+
await v.goto(address);
229+
const yours = v.getByRole('list', { name: 'Your files' });
230+
await v.getByTestId('request-picker').setInputFiles([{ name: 'waited.txt', mimeType: 'text/plain', buffer: Buffer.from('w') }]);
231+
await expect(yours.getByRole('listitem').filter({ hasText: 'waited.txt' })).toContainText('Sent', { timeout: 15_000 });
232+
expect(refusedAt).toHaveLength(1);
233+
234+
refuse = 3;
235+
await v.getByTestId('request-picker').setInputFiles([{ name: 'refused.txt', mimeType: 'text/plain', buffer: Buffer.from('r') }]);
236+
await expect(v.getByRole('alert')).toHaveText('Not sent: Too many files in a short time. Wait a minute and try again.', { timeout: 15_000 });
237+
expect(refusedAt).toHaveLength(4);
238+
// Each wait is the server's `retry-after` of 2 s.
239+
expect(refusedAt[3]! - refusedAt[1]!).toBeGreaterThanOrEqual(3900);
240+
await v.context().close();
241+
await cleanUp(page, dir);
242+
});
243+
163244
// #304: an unfinished upload of a visitor: the tus creation of HELD bytes and half of them, through the visitor's own
164245
// session. It holds HELD bytes of the owner's quota (half staged, half reserved).
165246
const HELD = 400_000;
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
-- A file request takes at most 1000 files (#401, `MAX_REQUEST_FILES`); a larger limit set before stands at the ceiling,
2+
-- so the owner sees the limit in force and can save the request again.
3+
UPDATE file_requests SET max_files = 1000 WHERE max_files > 1000;
4+
-- A visitor's file whose upload was dropped before it was placed (overrun, expired) counts nowhere; it went with the
5+
-- upload from now on (#401), and the ones left behind before go here.
6+
DELETE FROM file_request_uploads WHERE placed_at IS NULL AND upload_id IS NOT NULL AND upload_id NOT IN (SELECT id FROM uploads);

0 commit comments

Comments
 (0)