fix(fs): keep a home directory on its account's username (#3920)

A root was free to be renamed or moved, so it could be parked on a
username no account held yet. The next account to take that name
provisioned a second root at the same path, and from then on either row
could answer a lookup of it — new entries inherit their owner from
whichever row resolved as the parent, so one account's files were
created owned by the other, and renaming the parked root dragged the
other account's subtree along with it.

Rename and move now refuse a root, every username claim checks the home
path before the user row is written, and renameUserHome refuses to heal
onto a path another account holds.
This commit is contained in:
Daniel Salazar
2026-09-21 23:29:09 -07:00
committed by GitHub
parent 1261976088
commit d07ddeaaf5
7 changed files with 215 additions and 1 deletions
@@ -5423,6 +5423,35 @@ describe('AuthController user-protected mutations (validation paths)', () => {
}
});
it('change-username: 400 when the home path is already occupied, before the rename', async () => {
const { user, actor } = await makeUserAndActor();
// A name no account holds, whose home path a stray row does. This is
// the shape of legacy drift; it used to leave two roots at one path.
const parked = `p_${uniq()}`;
const root = (await server.stores.fsEntry.getRootEntryForUser(
user.id,
))!;
await server.clients.db.write(
'UPDATE fsentries SET path = ? WHERE id = ?',
[`/${parked}`, root.id],
);
const { user: mover, actor: moverActor } = await makeUserAndActor();
await expect(
controller.handleChangeUsername(
makeReq({ new_username: parked }, { actor: moverActor }),
makeRes(),
),
).rejects.toMatchObject({ statusCode: 400 });
// The username claim is refused whole: the user row keeps its name.
const after = await server.stores.user.getById(mover.id, {
force: true,
});
expect(after!.username).toBe(mover.username);
void actor;
});
it('change-email: 400 on missing/invalid email and on a confirmed-account collision', async () => {
const { actor } = await makeUserAndActor();
await expect(
@@ -836,6 +836,15 @@ export class AuthController extends PuterController {
);
}
// ...and the same against the filesystem: a free username whose home
// path is occupied would provision a second root there, and the two
// trees then resolve interchangeably.
if (await this.stores.fsEntry.findHomePathConflict(body.username)) {
throw new HttpError(400, 'This username is not available.', {
legacyCode: 'bad_request',
});
}
// Duplicate confirmed-email check. A confirmed account (any
// credential type — password OR OIDC) on this email → reject.
//
@@ -2587,6 +2596,19 @@ export class AuthController extends PuterController {
legacyCode: 'username_already_in_use',
});
}
// Before the username is written, not after: the rename below is what
// keeps the account's files reachable, and it can't run onto a taken
// path.
if (
await this.stores.fsEntry.findHomePathConflict(
new_username,
req.actor!.user.id!,
)
) {
throw new HttpError(400, 'This username is not available.', {
legacyCode: 'username_already_in_use',
});
}
await this.stores.user.update(req.actor!.user.id!, {
username: new_username,
@@ -2911,6 +2933,11 @@ export class AuthController extends PuterController {
legacyCode: 'username_already_in_use',
});
}
if (await this.stores.fsEntry.findHomePathConflict(username, user.id)) {
throw new HttpError(400, 'This username is not available.', {
legacyCode: 'username_already_in_use',
});
}
// Match raw + canonical to catch gmail-alias collisions, and
// reject on ANY confirmed account (OIDC accounts have
// password=null but are real) — not just password-holders.
+59
View File
@@ -2489,6 +2489,65 @@ describe('FSService remove', () => {
});
});
describe('FSService home directory guard', () => {
// A home free to be renamed or moved can be parked on a name another
// account later claims, and from then on both trees resolve at one path —
// whichever row has the lower id answers, and new entries there inherit
// ITS owner. Only `renameUserHome` writes a home's name.
it('refuses to rename a home directory', async () => {
const user = await makeUser();
const root = (await server.stores.fsEntry.getRootEntryForUser(
user.userId,
))!;
const error = await caught(() =>
fs.rename(user.userId, root, 'some-other-name'),
);
expect(error.statusCode).toBe(403);
expect(error.message).toContain('home directory');
const unchanged = await server.stores.fsEntry.getRootEntryForUser(
user.userId,
);
expect(unchanged?.path).toBe(user.home);
});
it('refuses to move a home directory into another tree', async () => {
const user = await makeUser();
const other = await makeUser();
const root = (await server.stores.fsEntry.getRootEntryForUser(
user.userId,
))!;
const destination = (await entryAt(other, '/Documents'))!;
const error = await caught(() =>
runWithContext({ actor: user.actor }, () =>
fs.move(user.userId, {
source: root,
destinationParent: destination,
}),
),
);
expect(error.statusCode).toBe(403);
const unchanged = await server.stores.fsEntry.getRootEntryForUser(
user.userId,
);
expect(unchanged?.path).toBe(user.home);
});
it('still renames an ordinary directory at the top of a home', async () => {
const user = await makeUser();
const dir = await fs.mkdir(user.userId, {
path: `${user.home}/notahome`,
});
const renamed = await fs.rename(user.userId, dir, 'renamed');
expect(renamed.path).toBe(`${user.home}/renamed`);
});
});
describe('FSService move', () => {
let user: TestUser;
beforeAll(async () => {
+21
View File
@@ -3464,6 +3464,7 @@ export class FSService extends PuterService {
entry: FSEntry,
newName: string,
): Promise<FSEntry> {
this.#assertNotUserRoot(entry, 'rename');
await this.#assertCanRename(entry, userId);
this.#assertUsableName(newName);
if (entry.name === newName) return entry;
@@ -3603,6 +3604,25 @@ export class FSService extends PuterService {
});
}
/**
* A home's name is the account's username, and ACL authorizes by matching
* the first path segment against it — so a root free to be renamed or moved
* can be parked on a name another account later claims, leaving two trees
* at one path. `renameUserHome` is the only writer of a home's name.
*
* Both parent columns have to be NULL: `parent_uid` alone is NULL on legacy
* rows that only ever carried `parent_id`.
*/
#assertNotUserRoot(entry: FSEntry, verb: string): void {
if (entry.parentUid === null && entry.parentId === null) {
throw new HttpError(
403,
`Cannot ${verb} a home directory — its name follows the account username.`,
{ legacyCode: 'forbidden' },
);
}
}
/**
* A FILE shared directly with you is renameable with `write` on it — the
* name is the file's own, and rename stays in place. A folder's name is
@@ -3996,6 +4016,7 @@ export class FSService extends PuterService {
},
): Promise<FSEntry> {
const { source, destinationParent } = input;
this.#assertNotUserRoot(source, 'move');
// The source only: moving *into* another app's AppData is a write, and
// ACL plus the fs:write class already cover that.
await this.#assertCrossAppDeleteAllowed(source.path);
@@ -1459,6 +1459,34 @@ describe('FSEntryStore home and prefix rewrites', () => {
).resolves.toBeNull();
});
it('refuses to heal a home onto a path another user holds', async () => {
const occupant = await makeUser();
const mover = await makeUser();
const conflict = await caught(() =>
store.renameUserHome(mover.userId, occupant.username),
);
expect(conflict.statusCode).toBe(409);
// The mover's home is untouched — no second row at that path.
const root = await store.getRootEntryForUser(mover.userId);
expect(root?.path).toBe(mover.home);
});
it('reports a home path conflict only for a foreign owner', async () => {
const owner = await makeUser();
const foreign = await store.findHomePathConflict(owner.username);
expect(foreign?.userId).toBe(owner.userId);
await expect(
store.findHomePathConflict(owner.username, owner.userId),
).resolves.toBeNull();
await expect(
store.findHomePathConflict('nobody-has-this-name'),
).resolves.toBeNull();
});
it('rewrites a path prefix and reports how many rows moved', async () => {
const user = await makeUser();
await store.ensureDirectoriesForUser(user.userId, [
+37 -1
View File
@@ -2608,7 +2608,8 @@ export class FSEntryStore extends PuterStore {
} = {},
): Promise<{ entries: FSEntry[]; cursor?: string }> {
const payload = decodeCursor(options.cursor) as
{ v: unknown; id: number; s?: string; o?: string } | undefined;
| { v: unknown; id: number; s?: string; o?: string }
| undefined;
const requestedSort = options.sortBy ?? null;
const requestedOrder = options.sortOrder ?? null;
@@ -3074,6 +3075,32 @@ export class FSEntryStore extends PuterStore {
return entry;
}
/**
* A row already sitting at `/{username}` that `ownerUserId` doesn't own.
* Two rows at one home path make each account's tree answer for the
* other's, so every point that claims a username checks this first. Reads
* the primary: a racing signup has to see the row just written.
*/
async findHomePathConflict(
username: string,
ownerUserId?: number,
): Promise<FSEntry | null> {
const path = this.#normalizePath(`/${username}`);
const rows = (await this.clients.db.pread(
`SELECT ${this.#selectFsentriesColumns()} FROM fsentries
WHERE path = ? ORDER BY id ASC`,
[path],
)) as unknown as FSEntryRow[];
for (const row of rows) {
const entry = this.#mapFSEntryRow(row);
if (ownerUserId !== undefined && entry.userId === ownerUserId) {
continue;
}
return entry;
}
return null;
}
// Heal a user's home tree to `/{username}`: if the root entry's path/name
// already match, no-op; otherwise rewrite the root row and cascade the
// prefix to descendants. Used by the username change flow AND by the
@@ -3091,6 +3118,15 @@ export class FSEntryStore extends PuterStore {
return root;
}
// Never heal onto a path someone else's row holds — the two would
// resolve interchangeably from then on.
const conflict = await this.findHomePathConflict(newUsername, userId);
if (conflict) {
throw new HttpError(409, `An entry already exists at ${newPath}`, {
legacyCode: 'conflict',
});
}
const oldPath = root.path;
const now = Math.floor(Date.now() / 1000);
+14
View File
@@ -19,6 +19,7 @@
import { v4 as uuidv4 } from 'uuid';
import type { AbstractDatabaseClient } from '../clients/database/DatabaseClient';
import { HttpError } from '../core/http/HttpError';
import type { GroupStore } from '../stores/group/GroupStore';
import type { UserRow, UserStore } from '../stores/user/UserStore';
import type { IConfig } from '../types';
@@ -58,6 +59,19 @@ export async function generateDefaultFsentries(
// Cheap check vs. a redundant INSERT + UPDATE on retries / re-runs.
if (user.trash_uuid) return;
// Callers check the name is free before provisioning, so this only fires
// on drift that predates those checks. A home written on top of another
// row gives both accounts a tree answering to one path.
const occupied = (await db.pread(
'SELECT id FROM fsentries WHERE path = ? LIMIT 1',
[`/${user.username}`],
)) as Array<{ id: number }>;
if (occupied.length > 0) {
throw new HttpError(400, 'This username is not available.', {
legacyCode: 'username_already_in_use',
});
}
const home_uuid = uuidv4();
const folderUuids: Record<FolderName, string> = {
Trash: uuidv4(),