fix: sharing answers "who can reach this" from the grants, not the index

Two ways a share listing could name access that was no longer there.

A recipient's listing paired the flat permission read with the list of
permissions it asked for by array index. That read drops misses and
dedupes its keys, so the two are not positional: one entry's live grant
vouched for another entry that had none, and listShared() kept
publishing a withdrawn item's name, size and signed thumbnail URL to
someone who could no longer open it. Read the permission off the value
instead.

getShares() had the same gap from the owner's side, with no liveness
check at all — a grant withdrawn through /auth/revoke-user-user or an
ACL mode change left the index row behind, and the owner was told
someone could reach a file they could not. Checked against the grants
now, one batched read per distinct holder. Pending invites are not
subject to it: they have no grant yet, which is the point of them.

Also covers the access-token actor, which reaches the same reach bound
as an app through a different arm of the ACL check. No behavior change
there — it was correct and untested.
This commit is contained in:
Juan Castro
2026-08-18 14:55:32 -04:00
parent b795b219a8
commit faea2a7760
2 changed files with 220 additions and 16 deletions
@@ -1251,6 +1251,97 @@ describe('ShareService', () => {
});
});
// The other derived actor: same reach bound, a different arm of the check.
describe('a token is bounded by what it was minted for', () => {
/** Mint a token and resolve it the way an authenticated request does. */
const asToken = async (
owner: { actor: Actor },
permissions: Array<[string]>,
) => {
const token = await runWithContext({ actor: owner.actor }, () =>
server.services.auth.createAccessToken(
owner.actor,
permissions,
{ label: 'share-test' },
),
);
const actor =
await server.services.auth.authenticateFromToken(token);
if (!actor) throw new Error('token did not resolve to an actor');
return actor;
};
it('shares a file the token carries', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const file = await makeFile(owner.user);
const actor = await asToken(owner, [[`fs:${file.uuid}:read`]]);
const result = await share(actor, {
uid: file.uuid,
recipient: { username: recipient.user.username },
mode: 'read',
});
expect(result.mode).toBe('read');
expect(await canRead(recipient.actor, file.path)).toBe(true);
});
it('refuses a file of its issuers that the token does not carry', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const carried = await makeFile(owner.user);
const other = await makeFile(owner.user);
const actor = await asToken(owner, [[`fs:${carried.uuid}:read`]]);
await expect(
share(actor, {
uid: other.uuid,
recipient: { username: recipient.user.username },
mode: 'read',
}),
).rejects.toMatchObject({ statusCode: 404 });
expect(await canRead(recipient.actor, other.path)).toBe(false);
});
it('cannot hand out more than it holds', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const file = await makeFile(owner.user);
const actor = await asToken(owner, [[`fs:${file.uuid}:read`]]);
await expect(
share(actor, {
uid: file.uuid,
recipient: { username: recipient.user.username },
mode: 'write',
}),
).rejects.toMatchObject({ statusCode: 403 });
expect(await canRead(recipient.actor, file.path)).toBe(false);
});
it('cannot withdraw a share on a file it does not carry', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const carried = await makeFile(owner.user);
const other = await makeFile(owner.user);
await share(owner.actor, {
uid: other.uuid,
recipient: { username: recipient.user.username },
mode: 'read',
});
const actor = await asToken(owner, [[`fs:${carried.uuid}:read`]]);
await expect(
unshare(actor, {
uid: other.uuid,
recipient: { username: recipient.user.username },
}),
).rejects.toMatchObject({ statusCode: 404 });
// The share it could not reach is still standing.
expect(await canRead(recipient.actor, other.path)).toBe(true);
});
});
it('retires grants when the entry is deleted', async () => {
const owner = await makeUser();
const recipient = await makeUser();
@@ -1489,6 +1580,65 @@ describe('ShareService', () => {
expect(after.items.map((i) => i.entryUid)).not.toContain(file.uuid);
});
// One entry's answer must not vouch for another's in the batched read.
it('drops a withdrawn listing even when another share survives', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const withdrawn = await makeFile(owner.user);
const kept = await makeFile(owner.user);
for (const file of [withdrawn, kept]) {
await share(owner.actor, {
uid: file.uuid,
recipient: { email: recipient.email },
mode: 'read',
});
}
await server.services.permission.revokeUserUserPermission(
owner.actor,
recipient.user.username!,
`fs:${withdrawn.uuid}:read`,
);
expect(await canRead(recipient.actor, withdrawn.path)).toBe(false);
expect(await canRead(recipient.actor, kept.path)).toBe(true);
const after = await server.services.share.listSharedWithMe(
recipient.actor,
);
const listed = after.items.map((i) => i.entryUid);
expect(listed).toContain(kept.uuid);
expect(listed).not.toContain(withdrawn.uuid);
});
// The owner's view of the same withdrawal.
it('stops naming a holder whose grant was withdrawn outside the index', async () => {
const owner = await makeUser();
const recipient = await makeUser();
const file = await makeFile(owner.user);
await share(owner.actor, {
uid: file.uuid,
recipient: { email: recipient.email },
mode: 'read',
});
await server.services.permission.revokeUserUserPermission(
owner.actor,
recipient.user.username!,
`fs:${file.uuid}:read`,
);
expect(await canRead(recipient.actor, file.path)).toBe(false);
const shares = await runWithContext({ actor: owner.actor }, () =>
server.services.share.listSharesOf(owner.actor, {
uid: file.uuid,
}),
);
expect(shares.map((s) => s.holder.username)).not.toContain(
recipient.user.username,
);
});
it('lets a recipient leave a share that was never indexed', async () => {
const owner = await makeUser();
const recipient = await makeUser();
+70 -16
View File
@@ -47,6 +47,17 @@ export interface ShareTarget {
uid?: string;
}
/** A `share` row, as much of it as this service reads back. */
interface ShareIndexRow {
uid: string;
mode: string;
holder_user_id: number;
issuer_user_id: number;
fsentry_id: number;
created_at?: unknown;
data?: unknown;
}
export interface ShareInput extends ShareTarget {
recipient: ShareRecipient;
mode: AclMode;
@@ -618,10 +629,10 @@ export class ShareService extends PuterService {
const uuid = uuidFromEntryPermission(row.permission);
if (uuid) live.add(uuid);
}
for (let i = 0; i < wanted.length; i++) {
const value = flat[i];
if (!value || value.deleted) continue;
const uuid = uuidFromEntryPermission(wanted[i]);
// Not positional against `wanted`: misses are dropped, keys deduped.
for (const value of flat) {
if (!value?.permission || value.deleted) continue;
const uuid = uuidFromEntryPermission(value.permission);
if (uuid) live.add(uuid);
}
// An owner listing something shared *to* them can't happen, but a
@@ -633,6 +644,37 @@ export class ShareService extends PuterService {
return live;
}
/** The `<holderId>:<fsentryId>` pairs whose grant is still standing. */
async #reachingHolders(
rows: ShareIndexRow[],
nodeById: Map<number, FSEntry>,
): Promise<Set<string>> {
const nodesByHolder = new Map<number, Map<number, FSEntry>>();
for (const row of rows) {
const holderId = Number(row.holder_user_id);
const node = nodeById.get(Number(row.fsentry_id));
if (!node || !Number.isFinite(holderId)) continue;
const nodes = nodesByHolder.get(holderId) ?? new Map();
nodes.set(node.id as number, node);
nodesByHolder.set(holderId, nodes);
}
const live = new Set<string>();
await Promise.all(
[...nodesByHolder].map(async ([holderId, nodes]) => {
const uuids = await this.#liveGrants(holderId, [
...nodes.values(),
]);
for (const node of nodes.values()) {
if (uuids.has(node.uuid)) {
live.add(`${holderId}:${node.id}`);
}
}
}),
);
return live;
}
/**
* Withdraw a recipient's access. An owner may clear any issuer's share of
* their node; anyone else may only clear the ones they issued.
@@ -986,13 +1028,15 @@ export class ShareService extends PuterService {
maskEntryPath(node),
]),
);
const inherited: Array<{ row: Record<string, unknown>; via: string }> =
(await this.stores.share.listByFsentries([...viaById.keys()])).map(
(row: { fsentry_id: number }) => ({
row,
via: viaById.get(Number(row.fsentry_id)) as string,
}),
);
const nodeById = new Map(
[entry, ...ancestorNodes.values()].map((node) => [node.id, node]),
);
const inherited: Array<{ row: ShareIndexRow; via: string }> = (
await this.stores.share.listByFsentries([...viaById.keys()])
).map((row: ShareIndexRow) => ({
row,
via: viaById.get(Number(row.fsentry_id)) as string,
}));
const rows = await this.stores.share.listByFsentry(entry.id);
const pendingRows = await this.stores.share.listPendingOnFsentry(
@@ -1012,8 +1056,19 @@ export class ShareService extends PuterService {
const users = await this.stores.user.getByIds(userIds);
const maskedPath = maskEntryPath(entry);
const inheritedShares: ResolvedShare[] = inherited.map(
({ row, via }) => ({
// As in `#liveGrants`: an index row outlives the grant it records.
const stillReaches = await this.#reachingHolders(
[...rows, ...inherited.map((i) => i.row)],
nodeById,
);
const isLive = (row: ShareIndexRow): boolean =>
stillReaches.has(
`${Number(row.holder_user_id)}:${Number(row.fsentry_id)}`,
);
const inheritedShares: ResolvedShare[] = inherited
.filter(({ row }) => isLive(row))
.map(({ row, via }) => ({
uid: String(row.uid),
mode: String(row.mode),
path: maskedPath,
@@ -1032,10 +1087,9 @@ export class ShareService extends PuterService {
inheritedFrom: via,
modified: entry.modified,
size: entry.size,
}),
);
}));
const own: ResolvedShare[] = rows.map(
const own: ResolvedShare[] = rows.filter(isLive).map(
(row: {
uid: string;
mode: string;