From 5b40092ae6ccbe48f0ebc8ca929118c3e4688aac Mon Sep 17 00:00:00 2001 From: ProgrammerIn-wonderland <3838shah@gmail.com> Date: Fri, 16 Oct 2026 14:00:28 -0400 Subject: [PATCH] PUT-2058 and PUT-2096 --- src/backend/controllers/fs/FSController.ts | 38 ++ .../controllers/fs/LegacyFSController.ts | 33 +- .../fs/scopedTokenWrites.http.test.ts | 354 ++++++++++++++++++ .../webdav/WebDAVController.test.ts | 281 +++++++++++++- .../controllers/webdav/WebDAVController.ts | 118 +++--- src/backend/services/acl/ACLService.test.ts | 34 ++ src/backend/services/acl/ACLService.ts | 10 +- 7 files changed, 793 insertions(+), 75 deletions(-) create mode 100644 src/backend/controllers/fs/scopedTokenWrites.http.test.ts diff --git a/src/backend/controllers/fs/FSController.ts b/src/backend/controllers/fs/FSController.ts index 873a24a7d..0d4c4616b 100644 --- a/src/backend/controllers/fs/FSController.ts +++ b/src/backend/controllers/fs/FSController.ts @@ -151,8 +151,14 @@ export class FSController extends PuterController { ); } + // Write routes admit scoped access tokens for the same reason the reads + // do (see the note above `/stat`): every handler authorizes the target + // through the ACL, which intersects a token's grant with its issuer's, so + // a token writes exactly where it was granted write — the parity WebDAV + // already has. Upload sessions are re-checked on complete, sign and abort. @Post('/startWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MULTIPART_LIMIT, @@ -219,6 +225,7 @@ export class FSController extends PuterController { @Post('/startBatchWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MULTIPART_LIMIT, @@ -325,6 +332,7 @@ export class FSController extends PuterController { @Post('/completeWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MULTIPART_LIMIT, @@ -367,6 +375,7 @@ export class FSController extends PuterController { @Post('/completeBatchWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MULTIPART_LIMIT, @@ -424,6 +433,7 @@ export class FSController extends PuterController { @Post('/abortWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MULTIPART_LIMIT, }) @@ -438,12 +448,31 @@ export class FSController extends PuterController { }); } + // An unknown id stays a no-op, as it always was. + try { + await this.services.fs.getUploadSessions(userId, [ + req.body.uploadId, + ]); + } catch (error) { + if (!(error instanceof HttpError && error.statusCode === 404)) { + throw error; + } + res.json({ ok: true }); + return; + } + // Same re-check as complete/sign: a scoped token may only abort an + // upload whose target it could write. + await this.#assertUploadSessionWriteAccess(req, userId, [ + req.body.uploadId, + ]); + await this.services.fs.abortUrlWrite(userId, req.body.uploadId); res.json({ ok: true }); } @Post('/signMultipartParts', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MULTIPART_LIMIT, @@ -476,6 +505,7 @@ export class FSController extends PuterController { @Post('/write', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_WRITE_LIMIT, @@ -531,6 +561,7 @@ export class FSController extends PuterController { @Post('/batchWrite', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_WRITE_LIMIT, @@ -1502,6 +1533,7 @@ export class FSController extends PuterController { @Post('/mkdir', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) @@ -1547,6 +1579,7 @@ export class FSController extends PuterController { @Post('/touch', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) @@ -1587,6 +1620,7 @@ export class FSController extends PuterController { @Post('/rename', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) @@ -1610,6 +1644,7 @@ export class FSController extends PuterController { @Post('/delete', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) @@ -1636,6 +1671,7 @@ export class FSController extends PuterController { @Post('/move', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) @@ -1689,6 +1725,7 @@ export class FSController extends PuterController { @Post('/copy', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, requireCredits: true, rateLimit: FS_MUTATE_LIMIT, @@ -1740,6 +1777,7 @@ export class FSController extends PuterController { @Post('/mkshortcut', { subdomain: 'api', + allowAccessToken: true, requireVerified: true, rateLimit: FS_MUTATE_LIMIT, }) diff --git a/src/backend/controllers/fs/LegacyFSController.ts b/src/backend/controllers/fs/LegacyFSController.ts index d8148b7c5..b512450bc 100644 --- a/src/backend/controllers/fs/LegacyFSController.ts +++ b/src/backend/controllers/fs/LegacyFSController.ts @@ -209,12 +209,19 @@ export class LegacyFSController extends PuterController { // Limits come from `./limits` and carry an explicit `scope`, so these // draw from the same per-user budget as their v2 counterparts rather // than handing a caller a second allowance for the same operation. - const mutate = { ...apiOptions, rateLimit: FS_MUTATE_LIMIT }; - // Read-side routes admit scoped access tokens: the ACL check each - // handler runs already intersects a token's grant with its issuer's, - // so the token reaches exactly what it was minted for. That is what a - // background events handler acts through — its `user` is a token - // scoped to the subscription's grant. + // + // The entry routes — reads and writes alike — admit scoped access + // tokens: the ACL check each handler runs already intersects a token's + // grant with its issuer's, so the token reaches exactly what it was + // minted for. That is what a background events handler acts through — + // its `user` is a token scoped to the subscription's grant — and it + // matches what WebDAV admits. Routes reaching past a single entry + // (search, df, sign, the app helpers) stay closed to them. + const mutate = { + ...apiOptions, + allowAccessToken: true, + rateLimit: FS_MUTATE_LIMIT, + } as RouteOptions; const reads = { ...apiOptions, allowAccessToken: true } as RouteOptions; router.post('/stat', { ...reads, rateLimit: FS_STAT_LIMIT }, this.stat); router.post( @@ -264,6 +271,8 @@ export class LegacyFSController extends PuterController { '/batch', { ...spends, + // Every op authorizes its own target. + allowAccessToken: true, rateLimit: FS_BATCH_LIMIT, concurrent: FS_BATCH_CONCURRENT, }, @@ -914,7 +923,9 @@ export class LegacyFSController extends PuterController { // Trash, and `null`/`{}` when restoring. See // `src/gui/src/helpers.js` → `window.move_items`. newMetadata: (body.new_metadata ?? undefined) as - Record | null | undefined, + | Record + | null + | undefined, }); const overwrittenEntry = replacedByMove(overwriteTarget, moved, name); const oldPath = source.path; @@ -1391,7 +1402,8 @@ export class LegacyFSController extends PuterController { } type SignedOrEmpty = - (SignedFile & { path?: string }) | Record; + | (SignedFile & { path?: string }) + | Record; const result: { signatures: SignedOrEmpty[]; token?: string } = { signatures: [], }; @@ -1986,7 +1998,10 @@ export class LegacyFSController extends PuterController { const subjectRef = body.subject; const appRef = body.app; const mode = (getString(body, 'mode') ?? 'read') as - 'see' | 'list' | 'read' | 'write'; + | 'see' + | 'list' + | 'read' + | 'write'; if (!subjectRef || !appRef) throw new HttpError(400, '`subject` and `app` are required', { legacyCode: 'bad_request', diff --git a/src/backend/controllers/fs/scopedTokenWrites.http.test.ts b/src/backend/controllers/fs/scopedTokenWrites.http.test.ts new file mode 100644 index 000000000..983c10234 --- /dev/null +++ b/src/backend/controllers/fs/scopedTokenWrites.http.test.ts @@ -0,0 +1,354 @@ +/* + * Copyright (C) 2024-present Puter Technologies Inc. + * + * This file is part of Puter. + * + * Puter is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { + createTestUser, + setupPuterTestEnv, + type PuterTestEnv, +} from '../../testUtil.js'; + +// A scoped access token writes through the `/fs` routes wherever its grant +// reaches — the reach it already has over WebDAV — and nowhere else. The ACL +// each handler runs intersects the token's grant with its issuer's, so these +// check both edges of that, plus the routes that stay closed to tokens. +describe('scoped access tokens on the /fs write routes', () => { + let env: PuterTestEnv; + + beforeAll(async () => { + env = await setupPuterTestEnv(); + }, 180_000); + + afterAll(async () => { + await env?.shutdown(); + }); + + const post = async (route: string, token: string, body: unknown) => { + const res = await fetch(new URL(route, env.apiOrigin), { + method: 'POST', + headers: { + 'content-type': 'application/json', + authorization: `Bearer ${token}`, + }, + body: JSON.stringify(body), + }); + const text = await res.text(); + let json: Record | null = null; + try { + json = text ? (JSON.parse(text) as Record) : null; + } catch { + // Some refusals are plain text. + } + return { status: res.status, json, text }; + }; + + /** A user, a token granted `mode` on their Documents, and some fixtures. */ + const setup = async (mode: 'read' | 'write') => { + const user = await createTestUser(env.server, { + username: `stw${Math.random().toString(36).slice(2, 9)}`, + password: 'puter-test-user-password', + }); + const home = `/${user.username}`; + const documents = await post('/stat', user.token, { + path: `${home}/Documents`, + }); + await post('/touch', user.token, { path: `${home}/Desktop/kept.txt` }); + await post('/touch', user.token, { path: `${home}/Documents/in.txt` }); + const minted = await post('/auth/create-access-token', user.token, { + permissions: [`fs:${documents.json!.uid}:${mode}`], + }); + expect(minted.status).toBe(200); + return { + user, + home, + token: minted.json!.token as string, + exists: async (path: string) => + (await post('/stat', user.token, { path })).status === 200, + }; + }; + + it('writes inside its grant on the legacy routes', async () => { + const { home, token, exists } = await setup('write'); + const docs = `${home}/Documents`; + + expect( + (await post('/mkdir', token, { path: `${docs}/d` })).status, + ).toBe(200); + expect( + (await post('/touch', token, { path: `${docs}/d/t.txt` })).status, + ).toBe(200); + const t = await post('/stat', token, { path: `${docs}/d/t.txt` }); + expect( + ( + await post('/rename', token, { + uid: t.json!.uid, + new_name: 'r.txt', + }) + ).status, + ).toBe(200); + expect( + ( + await post('/copy', token, { + source: `${docs}/d/r.txt`, + destination: docs, + }) + ).status, + ).toBe(200); + expect( + ( + await post('/move', token, { + source: `${docs}/r.txt`, + destination: `${docs}/d`, + new_name: 'm.txt', + }) + ).status, + ).toBe(200); + expect( + (await post('/delete', token, { paths: [`${docs}/d/m.txt`] })) + .status, + ).toBe(200); + const batch = await post('/batch', token, { + operations: [ + { op: 'mkdir', path: `${docs}/b` }, + { op: 'delete', path: `${docs}/d/r.txt` }, + ], + }); + expect(batch.status, batch.text).toBe(200); + + expect(await exists(`${docs}/b`)).toBe(true); + expect(await exists(`${docs}/d/r.txt`)).toBe(false); + expect(await exists(`${docs}/d/m.txt`)).toBe(false); + }, 180_000); + + it('writes inside its grant on the v2 routes', async () => { + const { home, token, exists } = await setup('write'); + const docs = `${home}/Documents`; + + expect( + (await post('/fs/mkdir', token, { path: `${docs}/v` })).status, + ).toBe(200); + expect( + (await post('/fs/touch', token, { path: `${docs}/v/t.txt` })) + .status, + ).toBe(200); + const written = await post('/fs/write', token, { + fileMetadata: { + path: `${docs}/v/w.txt`, + size: 3, + contentType: 'text/plain', + }, + fileContent: 'abc', + }); + expect(written.status, written.text).toBe(200); + expect( + ( + await post('/fs/rename', token, { + path: `${docs}/v/t.txt`, + new_name: 'r.txt', + }) + ).status, + ).toBe(200); + expect( + ( + await post('/fs/copy', token, { + source: `${docs}/v/r.txt`, + destination: docs, + }) + ).status, + ).toBe(200); + expect( + ( + await post('/fs/move', token, { + source: `${docs}/r.txt`, + destination: `${docs}/v`, + new_name: 'm.txt', + }) + ).status, + ).toBe(200); + expect( + ( + await post('/fs/mkshortcut', token, { + parent: docs, + target: `${docs}/v/w.txt`, + name: 'w-link', + }) + ).status, + ).toBe(200); + expect( + (await post('/fs/delete', token, { path: `${docs}/v/m.txt` })) + .status, + ).toBe(200); + const started = await post('/fs/startWrite', token, { + fileMetadata: { path: `${docs}/v/up.bin`, size: 4 }, + }); + expect(started.status, started.text).toBe(200); + expect( + ( + await post('/fs/abortWrite', token, { + uploadId: started.json!.sessionId, + }) + ).status, + ).toBe(200); + + expect(await exists(`${docs}/v/w.txt`)).toBe(true); + expect(await exists(`${docs}/w-link`)).toBe(true); + expect(await exists(`${docs}/v/m.txt`)).toBe(false); + }, 180_000); + + it('is refused everywhere outside its grant', async () => { + const { home, token, exists } = await setup('write'); + const docs = `${home}/Documents`; + const desktop = `${home}/Desktop`; + + const attempts = [ + ['/mkdir', { path: `${desktop}/x` }], + ['/touch', { path: `${desktop}/x.txt` }], + ['/move', { source: `${docs}/in.txt`, destination: desktop }], + ['/copy', { source: `${desktop}/kept.txt`, destination: docs }], + ['/delete', { paths: [`${desktop}/kept.txt`] }], + ['/delete', { paths: [home] }], + ['/fs/mkdir', { path: `${desktop}/x` }], + [ + '/fs/write', + { + fileMetadata: { + path: `${desktop}/kept.txt`, + size: 1, + overwrite: true, + }, + fileContent: 'x', + }, + ], + ['/fs/move', { source: `${docs}/in.txt`, destination: desktop }], + ['/fs/copy', { source: `${desktop}/kept.txt`, destination: docs }], + ['/fs/rename', { path: `${desktop}/kept.txt`, new_name: 'y.txt' }], + ['/fs/delete', { path: `${desktop}/kept.txt` }], + ['/fs/delete', { path: home }], + ] as const; + for (const [route, body] of attempts) { + const res = await post(route, token, body); + expect( + [403, 404], + `${route} ${JSON.stringify(body)} → ${res.status}`, + ).toContain(res.status); + } + + const batch = await post('/batch', token, { + operations: [{ op: 'mkdir', path: `${desktop}/b` }], + }); + expect(batch.status).not.toBe(200); + + expect(await exists(`${desktop}/kept.txt`)).toBe(true); + expect(await exists(`${desktop}/x`)).toBe(false); + expect(await exists(`${desktop}/b`)).toBe(false); + expect(await exists(`${docs}/in.txt`)).toBe(true); + expect(await exists(`${docs}/kept.txt`)).toBe(false); + }, 180_000); + + it('refuses every write to a read-only token', async () => { + const { home, token, exists } = await setup('read'); + const docs = `${home}/Documents`; + + const attempts = [ + ['/mkdir', { path: `${docs}/x` }], + ['/touch', { path: `${docs}/x.txt` }], + ['/delete', { paths: [`${docs}/in.txt`] }], + ['/fs/mkdir', { path: `${docs}/x` }], + [ + '/fs/write', + { + fileMetadata: { path: `${docs}/x.txt`, size: 1 }, + fileContent: 'x', + }, + ], + [ + '/fs/startWrite', + { fileMetadata: { path: `${docs}/x.bin`, size: 1 } }, + ], + ['/fs/rename', { path: `${docs}/in.txt`, new_name: 'y.txt' }], + ['/fs/delete', { path: `${docs}/in.txt` }], + ] as const; + for (const [route, body] of attempts) { + const res = await post(route, token, body); + expect(res.status, `${route} → ${res.status}`).toBe(403); + } + expect(await exists(`${docs}/in.txt`)).toBe(true); + expect(await exists(`${docs}/x`)).toBe(false); + }, 180_000); + + // An upload session belongs to the account, not to the credential that + // opened it, so each later step re-checks the target against the caller. + it('cannot touch an upload the session opened outside its grant', async () => { + const { user, home, token } = await setup('write'); + const started = await post('/fs/startWrite', user.token, { + fileMetadata: { path: `${home}/Desktop/up.bin`, size: 4 }, + }); + expect(started.status).toBe(200); + const uploadId = started.json!.sessionId as string; + + for (const [route, body] of [ + ['/fs/abortWrite', { uploadId }], + ['/fs/completeWrite', { uploadId }], + ['/fs/signMultipartParts', { uploadId, partNumbers: [1] }], + ] as const) { + const res = await post(route, token, body); + expect( + [403, 404], + `${route} → ${res.status} ${res.text}`, + ).toContain(res.status); + } + + // Still open: the session's own abort finds it to abort. + const sessions = await env.server.services.fs.getUploadSessions( + (await env.server.stores.user.getByUsername(user.username))!.id, + [uploadId], + ); + expect(sessions[0]?.status).not.toBe('aborted'); + expect( + (await post('/fs/abortWrite', user.token, { uploadId })).status, + ).toBe(200); + }, 180_000); + + it('aborting an unknown upload stays a no-op', async () => { + const { token } = await setup('write'); + const res = await post('/fs/abortWrite', token, { + uploadId: 'no-such-upload', + }); + expect(res.status).toBe(200); + }, 180_000); + + it('keeps the routes that reach past one entry closed', async () => { + const { home, token } = await setup('write'); + const docs = `${home}/Documents`; + + for (const [route, body] of [ + ['/sign', { items: [{ path: `${docs}/in.txt`, action: 'read' }] }], + ['/search', { text: 'in' }], + ['/fs/search', { query: 'in' }], + ['/df', {}], + ['/readdir-subdomains', {}], + ] as const) { + const res = await post(route, token, body); + expect(res.status, `${route} → ${res.status}`).toBe(403); + expect(res.json?.message ?? res.text).toMatch( + /Access tokens are not allowed/, + ); + } + }, 180_000); +}); diff --git a/src/backend/controllers/webdav/WebDAVController.test.ts b/src/backend/controllers/webdav/WebDAVController.test.ts index 169d5a74b..636abc476 100644 --- a/src/backend/controllers/webdav/WebDAVController.test.ts +++ b/src/backend/controllers/webdav/WebDAVController.test.ts @@ -1006,7 +1006,7 @@ describe('WebDAVController', () => { }); describe('DELETE', () => { - it('rejects delete when ACL denies write access', async () => { + it('refuses a delete outside the caller’s tree as not found', async () => { const { res, captured } = makeRes(); await dispatchMiddleware( makeReq({ @@ -1023,7 +1023,7 @@ describe('WebDAVController', () => { res, noop, ); - expect(captured.statusCode).toBe(403); + expect(captured.statusCode).toBe(404); }); }); @@ -1593,7 +1593,7 @@ describe('WebDAVController verbs', () => { expect(captured.body).toBe('Cannot GET a directory'); }); - it("denies reading another user's file with 403", async () => { + it("hides another user's file as not found", async () => { const owner = await makeUser(); const stranger = await makeUser(); const path = `/${owner.username}/Documents/private.txt`; @@ -1604,8 +1604,8 @@ describe('WebDAVController verbs', () => { path, actor: stranger.actor, }); - expect(captured.statusCode).toBe(403); - expect(captured.body).toBe('Permission denied'); + expect(captured.statusCode).toBe(404); + expect(captured.body).toBe('Not Found'); }); }); @@ -1925,7 +1925,8 @@ describe('WebDAVController verbs', () => { path: `/${owner.username}/Documents/intruder`, actor: stranger.actor, }); - expect(captured.statusCode).toBe(403); + // The parent is out of sight, so the refusal doesn't confirm it. + expect(captured.statusCode).toBe(404); }); }); @@ -2136,7 +2137,7 @@ describe('WebDAVController verbs', () => { }, }); expect(captured.statusCode).toBe(404); - expect(captured.body).toBe('Source not found'); + expect(captured.body).toBe('Not Found'); }); it('returns 409 when the destination parent is not a directory', async () => { @@ -2262,7 +2263,7 @@ describe('WebDAVController verbs', () => { }, }); expect(captured.statusCode).toBe(404); - expect(captured.body).toBe('Source not found'); + expect(captured.body).toBe('Not Found'); }); it("denies moving into another user's home", async () => { @@ -2280,7 +2281,7 @@ describe('WebDAVController verbs', () => { host: 'dav.puter.localhost', }, }); - expect(captured.statusCode).toBe(403); + expect(captured.statusCode).toBe(404); }); }); @@ -2579,7 +2580,7 @@ describe('WebDAVController verbs', () => { expect(xml).not.toContain('Documents'); }); - it('keeps a sharer’s collection a 403 for a caller with no share', async () => { + it('keeps a sharer’s collection out of sight for a caller with no share', async () => { const owner = await makeUser(); const stranger = await makeUser(); await shareFolder(owner, await makeUser()); @@ -2588,7 +2589,8 @@ describe('WebDAVController verbs', () => { `/${owner.username}`, stranger.actor, ); - expect(captured.statusCode).toBe(403); + // The same 404 as a name nobody holds, so it confirms no account. + expect(captured.statusCode).toBe(404); }); }); @@ -2758,9 +2760,9 @@ describe('WebDAVController verbs', () => { const root = await propfind('/', holder.actor); expect(String(root.body)).not.toContain(`/${owner.username}/`); - // And the level it used to stand in for is a plain 403 again. + // And the level it used to stand in for is out of sight again. const owned = await propfind(`/${owner.username}`, holder.actor); - expect(owned.statusCode).toBe(403); + expect(owned.statusCode).toBe(404); }); it('refuses writes and reads aimed at the invented levels', async () => { @@ -2809,4 +2811,257 @@ describe('WebDAVController verbs', () => { ).toBeFalsy(); }); }); + + // Every check runs on the path the entry is stored under, and a refusal of + // something the caller can't see reads exactly like a path with nothing + // at it. + describe('authorization against the stored entry', () => { + it('answers a stranger alike for a missing path and a private file', async () => { + const owner = await makeUser(); + const stranger = await makeUser(); + await putFile( + owner.actor, + `/${owner.username}/Documents/private.txt`, + 'secret', + ); + + const answers = []; + for (const [method, path] of [ + ['GET', `/${owner.username}/Documents/private.txt`], + ['GET', `/${owner.username}/Documents/missing.txt`], + // A directory too: refused before the "is a directory" 400. + ['GET', `/${owner.username}/Documents`], + ['PROPFIND', `/${owner.username}`], + ['PROPFIND', `/no-such-user-${uuidv4()}`], + ['DELETE', `/${owner.username}/Documents/private.txt`], + ['DELETE', `/${owner.username}/Documents/missing.txt`], + ] as const) { + const captured = await dispatch({ + method, + path, + actor: stranger.actor, + }); + answers.push([captured.statusCode, captured.body]); + } + for (const answer of answers) { + expect(answer).toEqual([404, 'Not Found']); + } + }); + + // MySQL collates `fsentries.path` case-insensitively, so a request for + // `//Public/...` can resolve a private `//public/...`. + // The test databases compare exactly, so the lookup is made to answer + // the way MySQL would; everything past it — the ancestor chain, the + // ACL — runs for real. + it('does not pass a case variant of Public off as the public folder', async () => { + const owner = await makeUser(); + const stranger = await makeUser(); + await server.stores.user.update(owner.userId, { + email_confirmed: 1, + }); + const privateDir = `/${owner.username}/public`; + const mkcol = await dispatch({ + method: 'MKCOL', + path: privateDir, + actor: owner.actor, + }); + expect(mkcol.statusCode).toBe(201); + await putFile(owner.actor, `${privateDir}/secret.txt`, 'secret'); + await putFile( + owner.actor, + `/${owner.username}/Public/open.txt`, + 'open', + ); + + const acl = server.services.acl as unknown as { + config: { enable_public_folders?: boolean }; + }; + const wasEnabled = acl.config.enable_public_folders; + acl.config.enable_public_folders = true; + try { + // The real Public folder is open to the stranger. + const open = await dispatch({ + method: 'GET', + path: `/${owner.username}/Public/open.txt`, + actor: stranger.actor, + }); + expect(open.statusCode).toBe(200); + + const store = server.stores.fsEntry; + const realLookup = store.getEntryByPath.bind(store); + const asked = `/${owner.username}/Public`; + vi.spyOn(store, 'getEntryByPath').mockImplementation( + (path: string) => + realLookup( + path === asked || path.startsWith(`${asked}/`) + ? privateDir + path.slice(asked.length) + : path, + ), + ); + + const { res, captured } = makeStreamRes(); + await dispatchMiddleware( + makeReq({ + method: 'GET', + path: `${asked}/secret.txt`, + actor: stranger.actor, + }), + res, + noop, + ); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(captured.statusCode).toBe(404); + expect(captured.text()).toBe(''); + + const listing = await dispatch({ + method: 'PROPFIND', + path: asked, + actor: stranger.actor, + }); + expect(listing.statusCode).toBe(404); + expect(String(listing.body)).not.toContain('secret.txt'); + + const copied = await dispatch({ + method: 'COPY', + path: `${asked}/secret.txt`, + actor: stranger.actor, + headers: { + destination: `/${stranger.username}/Documents/loot.txt`, + host: 'dav.puter.localhost', + }, + }); + expect(copied.statusCode).toBe(404); + } finally { + vi.restoreAllMocks(); + acl.config.enable_public_folders = wasEnabled; + } + expect( + await server.stores.fsEntry.getEntryByPath( + `/${stranger.username}/Documents/loot.txt`, + ), + ).toBeFalsy(); + }); + }); + + // A scoped token writes over DAV wherever its grant reaches, and nowhere + // else — the same reach it has over the `/fs` write routes. + describe('scoped access tokens', () => { + const mintFor = async (userId: number, permission: string) => { + const user = (await server.stores.user.getById(userId))!; + return server.services.auth.createAccessToken(makeActor({ user }), [ + [permission], + ]); + }; + // Own address, so the per-IP auth failure bucket can't refuse these. + const asToken = (token: string, init: Parameters[0]) => + dispatch({ + ...init, + ip: '203.0.113.58', + headers: { + ...init.headers, + authorization: basicAuth('-token', token), + }, + }); + + it('writes inside its grant and is refused outside it', async () => { + const { userId, username, actor } = await makeUser(); + const documents = (await server.stores.fsEntry.getEntryByPath( + `/${username}/Documents`, + ))!; + const token = await mintFor(userId, `fs:${documents.uuid}:write`); + const inside = `/${username}/Documents/tok`; + + expect( + (await asToken(token, { method: 'MKCOL', path: inside })) + .statusCode, + ).toBe(201); + expect( + ( + await asToken(token, { + method: 'PUT', + path: `${inside}/a.txt`, + headers: { 'content-length': '3' }, + content: 'abc', + }) + ).statusCode, + ).toBe(201); + expect( + ( + await asToken(token, { + method: 'MOVE', + path: `${inside}/a.txt`, + headers: { + destination: `${inside}/b.txt`, + host: 'dav.puter.localhost', + }, + }) + ).statusCode, + ).toBe(201); + expect( + ( + await asToken(token, { + method: 'DELETE', + path: `${inside}/b.txt`, + }) + ).statusCode, + ).toBe(204); + + await putFile(actor, `/${username}/Desktop/kept.txt`, 'kept'); + for (const init of [ + { method: 'MKCOL', path: `/${username}/Desktop/intruder` }, + { + method: 'PUT', + path: `/${username}/Desktop/kept.txt`, + headers: { 'content-length': '1' }, + content: 'x', + }, + { method: 'DELETE', path: `/${username}/Desktop/kept.txt` }, + { + method: 'COPY', + path: `/${username}/Desktop/kept.txt`, + headers: { + destination: `${inside}/kept.txt`, + host: 'dav.puter.localhost', + }, + }, + { method: 'DELETE', path: `/${username}` }, + ]) { + const captured = await asToken(token, init); + // Outside the grant is out of sight, so it reads as missing. + expect(captured.statusCode, init.method).toBe(404); + } + expect( + await server.stores.fsEntry.getEntryByPath( + `/${username}/Desktop/intruder`, + ), + ).toBeFalsy(); + expect( + await server.stores.fsEntry.getEntryByPath( + `/${username}/Desktop/kept.txt`, + ), + ).toMatchObject({ size: 4 }); + }); + + it('refuses every write to a read-only token', async () => { + const { userId, username } = await makeUser(); + const documents = (await server.stores.fsEntry.getEntryByPath( + `/${username}/Documents`, + ))!; + const token = await mintFor(userId, `fs:${documents.uuid}:read`); + + for (const init of [ + { method: 'MKCOL', path: `/${username}/Documents/ro` }, + { + method: 'PUT', + path: `/${username}/Documents/ro.txt`, + headers: { 'content-length': '1' }, + content: 'x', + }, + { method: 'DELETE', path: `/${username}/Documents` }, + ]) { + const captured = await asToken(token, init); + expect(captured.statusCode, init.method).toBe(403); + } + }); + }); }); diff --git a/src/backend/controllers/webdav/WebDAVController.ts b/src/backend/controllers/webdav/WebDAVController.ts index 559d3e37d..53d32fbca 100644 --- a/src/backend/controllers/webdav/WebDAVController.ts +++ b/src/backend/controllers/webdav/WebDAVController.ts @@ -514,15 +514,15 @@ export class WebDAVController extends PuterController { headOnly: boolean, ): Promise { const entry = await this.stores.fsEntry.getEntryByPath(davPath); - if (!entry) - throw new HttpError(404, 'Not Found', { legacyCode: 'not_found' }); + if (!entry) throw davNotFound(); + // Authorized before the directory check, so a caller who can't see + // the entry can't learn that it is one. + await this.#assertAccess(actor, entry.path, 'read'); if (entry.isDir) throw new HttpError(400, 'Cannot GET a directory', { legacyCode: 'bad_request', }); - await this.#assertRead(actor, davPath); - const modified = entry.modified ?? entry.created ?? 0; const etag = entryEtag(entry.uuid, modified); const size = entry.size ?? 0; @@ -568,7 +568,7 @@ export class WebDAVController extends PuterController { const depth = req.headers.depth ?? '1'; if (davPath === '/') { - await this.#assertRead(actor, davPath); + await this.#assertAccess(actor, davPath, 'read'); this.#sendMultistatus(res, await this.#rootPropfind(actor, depth)); return; } @@ -590,11 +590,9 @@ export class WebDAVController extends PuterController { return; } - if (!entry) { - throw new HttpError(404, 'Not Found', { legacyCode: 'not_found' }); - } + if (!entry) throw davNotFound(); - await this.#assertRead(actor, davPath); + await this.#assertAccess(actor, entry.path, 'read'); // `davPath` is the resolved real path; the href has to be the masked // one, like every child below it, or the self-entry names the owner's @@ -696,7 +694,7 @@ export class WebDAVController extends PuterController { if (!entry?.isDir) return null; // An owner who shared their whole home directory has a real listing // here, and it beats the virtual one. - if (await this.#canRead(actor, davPath)) return null; + if (await this.#canAccess(actor, entry.path, 'read')) return null; const shares = (await this.#sharedWithMe(actor)).filter( (share) => share.owner?.username === owner, @@ -787,8 +785,7 @@ export class WebDAVController extends PuterController { throw new HttpError(423, 'Locked', { legacyCode: 'conflict' }); } const userId = actor.user!.id as number; - const parentPath = pathPosix.dirname(davPath); - await this.#assertWrite(actor, parentPath); + await this.#assertCreate(actor, davPath); const existing = await this.stores.fsEntry.getEntryByPath(davPath); if (existing) @@ -824,8 +821,11 @@ export class WebDAVController extends PuterController { } const userId = actor.user!.id as number; - const parentPath = pathPosix.dirname(davPath); - await this.#assertWrite(actor, parentPath); + // Overwriting takes write on what is there, as `/fs/write` does; + // creating takes write on the parent. + const existing = await this.stores.fsEntry.getEntryByPath(davPath); + if (existing) await this.#assertAccess(actor, existing.path, 'write'); + else await this.#assertCreate(actor, davPath); const contentLength = Number( req.headers['content-length'] ?? @@ -837,9 +837,6 @@ export class WebDAVController extends PuterController { legacyCode: 'bad_request', }); - // Check if overwrite - const existing = await this.stores.fsEntry.getEntryByPath(davPath); - // Expect: 100-continue if (req.headers.expect === '100-continue') { (req.socket as { write?: (s: string) => void }).write?.( @@ -884,11 +881,9 @@ export class WebDAVController extends PuterController { throw new HttpError(423, 'Locked', { legacyCode: 'conflict' }); } const userId = actor.user!.id as number; - await this.#assertWrite(actor, davPath); - const entry = await this.stores.fsEntry.getEntryByPath(davPath); - if (!entry) - throw new HttpError(404, 'Not Found', { legacyCode: 'not_found' }); + if (!entry) throw davNotFound(); + await this.#assertAccess(actor, entry.path, 'write'); await this.services.fs.remove(userId, { entry, recursive: true }); this.#emitGuiEvent('outer.gui.item.removed', entry); @@ -910,14 +905,10 @@ export class WebDAVController extends PuterController { } const userId = actor.user!.id as number; - await this.#assertRead(actor, davPath); - await this.#assertWrite(actor, pathPosix.dirname(destPath)); - const source = await this.stores.fsEntry.getEntryByPath(davPath); - if (!source) - throw new HttpError(404, 'Source not found', { - legacyCode: 'not_found', - }); + if (!source) throw davNotFound(); + await this.#assertAccess(actor, source.path, 'read'); + await this.#assertCreate(actor, destPath); const overwrite = req.headers.overwrite !== 'F'; const destExists = await this.stores.fsEntry.getEntryByPath(destPath); @@ -962,14 +953,10 @@ export class WebDAVController extends PuterController { throw new HttpError(423, 'Locked', { legacyCode: 'conflict' }); const userId = actor.user!.id as number; - await this.#assertWrite(actor, davPath); - await this.#assertWrite(actor, pathPosix.dirname(destPath)); - const source = await this.stores.fsEntry.getEntryByPath(davPath); - if (!source) - throw new HttpError(404, 'Source not found', { - legacyCode: 'not_found', - }); + if (!source) throw davNotFound(); + await this.#assertAccess(actor, source.path, 'write'); + await this.#assertCreate(actor, destPath); const overwrite = req.headers.overwrite !== 'F'; const destExists = await this.stores.fsEntry.getEntryByPath(destPath); @@ -1014,8 +1001,11 @@ export class WebDAVController extends PuterController { // ACL must succeed before any lock state is touched — otherwise // an authenticated user could lock paths they don't own (e.g. `/`) - // and block writes for everyone else. - await this.#assertWrite(actor, davPath); + // and block writes for everyone else. A path with nothing at it yet + // is a lock-null resource, locked ahead of its create. + const target = await this.stores.fsEntry.getEntryByPath(davPath); + if (target) await this.#assertAccess(actor, target.path, 'write'); + else await this.#assertCreate(actor, davPath); const timeoutSeconds = parseLockTimeout( req.headers.timeout as string | undefined, @@ -1148,25 +1138,46 @@ export class WebDAVController extends PuterController { // -- ACL helpers ------------------------------------------------- - async #canRead(actor: Actor, path: string): Promise { + // Checks take the path an entry is stored under, never the request's + // spelling of it: the path lookup can match a differently-cased row (MySQL + // collates `fsentries.path` case-insensitively), while the ACL's path rules + // — home, Public — compare exactly. Only a create, whose target has no row + // yet, falls back to a requested path. + + async #canAccess( + actor: Actor, + path: string, + mode: 'read' | 'write', + ): Promise { const acl = this.services.acl; - return await acl.check(actor, acl.fsDescriptor(path), 'read'); + return await acl.check(actor, acl.fsDescriptor(path), mode); } - async #assertRead(actor: Actor, path: string): Promise { - if (!(await this.#canRead(actor, path))) - throw new HttpError(403, 'Permission denied', { - legacyCode: 'permission_denied', - }); + /** + * Refused the way `/fs` refuses: a caller who can't even see the target + * gets the same 404 as for a path with nothing at it, so a refusal never + * confirms that something exists. + */ + async #assertAccess( + actor: Actor, + path: string, + mode: 'read' | 'write', + ): Promise { + const acl = this.services.acl; + const descriptor = acl.fsDescriptor(path); + if (await acl.check(actor, descriptor, mode)) return; + const safe = await acl.getSafeAclError(actor, descriptor, mode); + if (safe.status === 404) throw davNotFound(); + throw new HttpError(403, 'Permission denied', { + legacyCode: 'permission_denied', + }); } - async #assertWrite(actor: Actor, path: string): Promise { - const acl = this.services.acl; - const ok = await acl.check(actor, acl.fsDescriptor(path), 'write'); - if (!ok) - throw new HttpError(403, 'Permission denied', { - legacyCode: 'permission_denied', - }); + /** Write on the parent of a path about to be created at `targetPath`. */ + async #assertCreate(actor: Actor, targetPath: string): Promise { + const parentPath = pathPosix.dirname(targetPath); + const parent = await this.stores.fsEntry.getEntryByPath(parentPath); + await this.#assertAccess(actor, parent?.path ?? parentPath, 'write'); } // -- Event emission ---------------------------------------------- @@ -1260,6 +1271,11 @@ function sendDavError(res: Response, err: unknown): void { res.status(500).send('Internal Server Error'); } +/** Nothing there — or nothing the caller may know is there; they read alike. */ +function davNotFound(): HttpError { + return new HttpError(404, 'Not Found', { legacyCode: 'not_found' }); +} + // -- XML helpers ------------------------------------------------------ function escapeXml(text: string): string { diff --git a/src/backend/services/acl/ACLService.test.ts b/src/backend/services/acl/ACLService.test.ts index 506837035..138301df3 100644 --- a/src/backend/services/acl/ACLService.test.ts +++ b/src/backend/services/acl/ACLService.test.ts @@ -552,6 +552,40 @@ describe('ACLService.check — public folders', () => { ).toBe(false); }); + it('stays closed for a case variant that resolved a private folder', async () => { + const { service, stores } = publicService(); + stores.user.getByUsername.mockResolvedValue({ + username: 'owner', + email_confirmed: true, + }); + // What MySQL hands back for `/owner/Public/...` when the owner's + // folder is stored as `public`: the lookup collates case-insensitively. + const variant: ResourceDescriptor = { + path: '/owner/Public/secret.txt', + resolveAncestors: async () => [ + { uid: 'uid:secret', path: '/owner/public/secret.txt' }, + { uid: 'uid:public', path: '/owner/public' }, + { uid: 'uid:owner', path: '/owner' }, + ], + }; + expect(await service.check(issuerActor, variant, 'read')).toBe(false); + }); + + it('stays closed when no Public folder is stored under the path', async () => { + const { service, stores } = publicService(); + stores.user.getByUsername.mockResolvedValue({ + username: 'owner', + email_confirmed: true, + }); + const orphan: ResourceDescriptor = { + path: '/owner/Public/index.html', + resolveAncestors: async () => [ + { uid: 'uid:owner', path: '/owner' }, + ], + }; + expect(await service.check(issuerActor, orphan, 'read')).toBe(false); + }); + it('stays closed when the feature flag is off', async () => { const { service, stores } = makeService(); stores.user.getByUsername.mockResolvedValue({ diff --git a/src/backend/services/acl/ACLService.ts b/src/backend/services/acl/ACLService.ts index e30de7a8d..64b6a9d83 100644 --- a/src/backend/services/acl/ACLService.ts +++ b/src/backend/services/acl/ACLService.ts @@ -220,12 +220,18 @@ export class ACLService extends PuterService { } // Public folders: //Public with read-ish mode, owner must have - // confirmed email (or be admin). + // confirmed email (or be admin). The folder has to be stored under + // exactly that name: a path lookup collates case-insensitively, so + // `//Public/...` can resolve a private `//public`, and the + // asked path alone would pass it as public. if ( this.config.enable_public_folders && (PUBLIC_READ_MODES as AclMode[]).includes(mode) && components.length > 1 && - components[1] === 'Public' + components[1] === 'Public' && + ancestors.some( + (ancestor) => ancestor.path === `/${components[0]}/Public`, + ) ) { const ownerUsername = components[0]; const owner = await this.stores.user.getByUsername(ownerUsername);