fix(security): scope the filesystem path picker per user in multi-user mode

Both picker endpoints are a second file-serving surface, and they
inherited neither the attachment guard's confinement nor its ownership
scoping. Two separate holes:

1. `sessionId` contributes that session's workingDir as a browse root,
   but it was resolved straight off ctx.sessions/ctx.store with no owner
   check, unlike the nine other session-scoped handlers in this file. A
   non-admin could pin ANOTHER user's working directory as a root just
   by passing their session id, then list and preview underneath it. Now
   runs canAccessOwned and reports 404, which also avoids confirming
   that a session id exists.

2. `Home` and `CASES_DIR` were unconditional roots for every caller.
   Per-user spaces live at <USER_SPACES_DIR>/<username>, which is INSIDE
   homedir(), so the Home root alone exposed every other user's
   workspace. A multi-user non-admin now gets only their own
   userSpacePath plus anything explicitly listed in
   CODEMAN_FILE_PICKER_ROOTS. /mnt/d is dropped as well: a broad host
   mount should be an explicit operator decision in a multi-user
   deployment, and operators who want it can name it in that env var.

Admins and single-user mode keep the host-wide roots, so behavior is
unchanged unless CODEMAN_MULTIUSER is on (opt-in, off by default).

All three discriminating tests were verified to fail against the
previous code: browse and preview both returned 200 instead of 404, and
the roots came back as [Home, Codeman Cases, ...] instead of [My Space].
Full suite green, 3784 passed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
2026-07-28 10:49:49 +02:00
parent bc232e5ff3
commit 3cff98fe56
6 changed files with 183 additions and 19 deletions
+12 -2
View File
@@ -20,18 +20,28 @@ export interface RouteTestHarness {
* @param registerFn - The route registration function (e.g., registerSessionRoutes).
* Uses `any` for ctx parameter because route functions expect typed port intersections
* that MockRouteContext satisfies structurally but not nominally.
* @param ctxOptions - Optional overrides for the mock context
* @param ctxOptions - Optional overrides for the mock context. `authUser` stands
* in for what the auth middleware would attach in multi-user mode; without it
* `getAuthUser()` falls back to a synthetic admin, which passes every
* ownership check and would make a scoping test pass vacuously.
*/
export async function createRouteTestHarness(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
registerFn: (app: FastifyInstance, ctx: any) => void,
ctxOptions?: { sessionId?: string }
ctxOptions?: { sessionId?: string; authUser?: { username: string; role: 'admin' | 'user' } }
): Promise<RouteTestHarness> {
const app = Fastify({ logger: false });
// Register cookie plugin — some routes access req.cookies
await app.register(fastifyCookie);
if (ctxOptions?.authUser) {
const authUser = ctxOptions.authUser;
app.addHook('onRequest', async (req) => {
(req as unknown as { authUser: typeof authUser }).authUser = authUser;
});
}
const ctx = createMockRouteContext(ctxOptions);
registerFn(app, ctx);
+112
View File
@@ -169,6 +169,118 @@ describe('file-routes', () => {
});
});
// ========== Multi-user scoping for the filesystem picker ==========
//
// The picker is a SECOND file-serving surface and does not inherit the
// attachment guard's ownership scoping, so both of its endpoints have to do
// it themselves. Two distinct holes are covered here:
// 1. `sessionId` was used without an owner check, so any user could pin
// another user's workingDir as a browse root.
// 2. `Home` and `CASES_DIR` were unconditional roots, and per-user spaces
// live INSIDE homedir(), so Home alone exposed every other user's files.
describe('filesystem picker multi-user scoping', () => {
const SPACES = '/tmp/codeman-test-user-spaces';
let prevMultiUser: string | undefined;
let prevSpaces: string | undefined;
beforeEach(() => {
prevMultiUser = process.env.CODEMAN_MULTIUSER;
prevSpaces = process.env.CODEMAN_USER_SPACES_DIR;
process.env.CODEMAN_MULTIUSER = '1';
process.env.CODEMAN_USER_SPACES_DIR = SPACES;
});
afterEach(() => {
if (prevMultiUser === undefined) delete process.env.CODEMAN_MULTIUSER;
else process.env.CODEMAN_MULTIUSER = prevMultiUser;
if (prevSpaces === undefined) delete process.env.CODEMAN_USER_SPACES_DIR;
else process.env.CODEMAN_USER_SPACES_DIR = prevSpaces;
});
const harnessAs = (role: 'admin' | 'user', username: string) =>
createRouteTestHarness(registerFileRoutes, { authUser: { username, role } });
it('404s a browse scoped to another user session instead of adopting its folder', async () => {
const scoped = await harnessAs('user', 'bob');
scoped.ctx._session.owner = 'alice';
try {
const res = await scoped.app.inject({
method: 'GET',
url: `/api/filesystem/browse?sessionId=${scoped.ctx._sessionId}`,
});
expect(res.statusCode).toBe(404);
expect(JSON.parse(res.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.NOT_FOUND });
// The decisive part: alice's folder must not have leaked in as a root.
expect(res.body).not.toContain(scoped.ctx._session.workingDir);
} finally {
await scoped.app.close();
}
});
it('404s a preview scoped to another user session', async () => {
const scoped = await harnessAs('user', 'bob');
scoped.ctx._session.owner = 'alice';
try {
const res = await scoped.app.inject({
method: 'GET',
url: `/api/filesystem/preview?sessionId=${scoped.ctx._sessionId}&path=${encodeURIComponent(
`${scoped.ctx._session.workingDir}/notes.md`
)}`,
});
expect(res.statusCode).toBe(404);
} finally {
await scoped.app.close();
}
});
it('confines a regular user to their own space, never Home or the shared cases dir', async () => {
const scoped = await harnessAs('user', 'bob');
try {
mockedReaddir.mockResolvedValueOnce([] as never);
const res = await scoped.app.inject({ method: 'GET', url: '/api/filesystem/browse' });
expect(res.statusCode).toBe(200);
const body = JSON.parse(res.body);
expect(body.data.roots).toEqual([{ label: 'My Space', path: `${SPACES}/bob` }]);
expect(body.data.path).toBe(`${SPACES}/bob`);
} finally {
await scoped.app.close();
}
});
it("refuses to browse another user's space by absolute path", async () => {
const scoped = await harnessAs('user', 'bob');
try {
const res = await scoped.app.inject({
method: 'GET',
url: `/api/filesystem/browse?path=${encodeURIComponent(`${SPACES}/alice/cases`)}`,
});
expect(res.statusCode).toBe(403);
expect(JSON.parse(res.body)).toMatchObject({ success: false, errorCode: ApiErrorCode.INVALID_INPUT });
} finally {
await scoped.app.close();
}
});
it('keeps the host-wide roots for a multi-user admin', async () => {
const scoped = await harnessAs('admin', 'root');
try {
mockedReaddir.mockResolvedValueOnce([] as never);
const res = await scoped.app.inject({ method: 'GET', url: '/api/filesystem/browse' });
expect(res.statusCode).toBe(200);
const labels = JSON.parse(res.body).data.roots.map((root: { label: string }) => root.label);
expect(labels).toContain('Home');
expect(labels).not.toContain('My Space');
} finally {
await scoped.app.close();
}
});
});
// ========== GET /api/filesystem/preview ==========
describe('GET /api/filesystem/preview', () => {