From 1ecddf3cbb4a86f7d81ebeb210893a35f03baa78 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:47:41 +0200 Subject: [PATCH] fix: copy whole folders and write to shared drives in Files Copying a folder sent one FileNode with no content, which Stalwart 0.16.0 rejected ("size") and 0.16.6+ turned into an empty folder. Copy now recreates the subtree level by level, and copies into another account move the file contents with `Blob/copy`. Every write (new folder, upload, rename, move, delete, copy, share) went to the primary account, including for nodes of a shared drive, which the listing names ":". Inside a shared drive the server read those ids as local ones and failed or touched the wrong node. Writes now go to the account that owns the node, with the bare id, and a move between accounts is refused instead of being sent. Creates pass `onExists: "rename"`, so an upload or new folder that clashes with an existing name becomes "name (2)" instead of failing. Stalwart before 0.16.6 ignores that argument and refuses the clash, so the client retries with a numbered name there. --- lib/__tests__/jmap-filenode-writes.test.ts | 102 ++++++ lib/jmap/client.ts | 353 +++++++++++++-------- 2 files changed, 326 insertions(+), 129 deletions(-) create mode 100644 lib/__tests__/jmap-filenode-writes.test.ts diff --git a/lib/__tests__/jmap-filenode-writes.test.ts b/lib/__tests__/jmap-filenode-writes.test.ts new file mode 100644 index 000000000..ebcf4aaee --- /dev/null +++ b/lib/__tests__/jmap-filenode-writes.test.ts @@ -0,0 +1,102 @@ +import { describe, it, expect, vi } from 'vitest'; +import { JMAPClient } from '../jmap/client'; + +type Call = [string, Record, string]; + +// A primary account "me" plus a shared drive "grp", as listed by +// listAllFileNodesAcrossAccounts ("grp:"). +function makeClient(nodes: Record>>) { + const client = new JMAPClient('https://jmap.example.com', 'user@example.com', 'pass'); + const fileCap = { 'urn:ietf:params:jmap:filenode': {} }; + Object.assign(client, { + accountId: 'me', + capabilities: { 'urn:ietf:params:jmap:filenode': {} }, + accounts: { me: { accountCapabilities: fileCap, isPersonal: true }, grp: { accountCapabilities: fileCap, isPersonal: false } }, + session: { + primaryAccounts: { 'urn:ietf:params:jmap:filenode': 'me' }, + accounts: { me: { accountCapabilities: fileCap, isPersonal: true }, grp: { accountCapabilities: fileCap, isPersonal: false } }, + }, + }); + vi.spyOn(client as unknown as { getFilesCapableAccountIds: () => string[] }, 'getFilesCapableAccountIds') + .mockReturnValue(['me', 'grp']); + const calls: Call[] = []; + let seq = 0; + vi.spyOn(client as unknown as { request: (c: Call[]) => Promise }, 'request') + .mockImplementation(async (reqCalls: Call[]) => { + const [method, args, id] = reqCalls[0]; + calls.push(reqCalls[0]); + if (method === 'FileNode/get') { + return { methodResponses: [['FileNode/get', { list: nodes[args.accountId as string] ?? [] }, id]] }; + } + if (method === 'Blob/copy') { + const blobIds = args.blobIds as string[]; + return { methodResponses: [['Blob/copy', { copied: Object.fromEntries(blobIds.map(b => [b, `${b}-copy`])) }, id]] }; + } + if (method === 'FileNode/set' && args.create) { + return { methodResponses: [['FileNode/set', { created: { n0: { id: `new${++seq}` } } }, id]] }; + } + if (method === 'FileNode/set' && args.update) { + return { methodResponses: [['FileNode/set', { updated: Object.fromEntries(Object.keys(args.update as object).map(k => [k, null])) }, id]] }; + } + if (method === 'FileNode/set' && args.destroy) { + return { methodResponses: [['FileNode/set', { destroyed: args.destroy }, id]] }; + } + throw new Error(`unexpected ${method}`); + }); + return { client, calls }; +} + +const sets = (calls: Call[]) => calls.filter(([m]) => m === 'FileNode/set').map(([, a]) => a); + +describe('FileNode writes', () => { + it('writes inside a shared drive go to that account with bare ids', async () => { + const { client, calls } = makeClient({}); + const dir = await client.createFileDirectory('New', 'grp:f1'); + await client.updateFileNode('grp:n1', { name: 'Renamed', parentId: 'grp:f2' }); + await client.destroyFileNodes(['grp:n1', 'n9']); + + expect(dir.id).toBe('grp:new1'); + const [create, update, destroyGrp, destroyMe] = sets(calls); + expect(create).toMatchObject({ accountId: 'grp', onExists: 'rename', create: { n0: { name: 'New', parentId: 'f1' } } }); + expect(update).toMatchObject({ accountId: 'grp', update: { n1: { name: 'Renamed', parentId: 'f2' } } }); + expect(destroyGrp).toMatchObject({ accountId: 'grp', destroy: ['n1'] }); + expect(destroyMe).toMatchObject({ accountId: 'me', destroy: ['n9'] }); + }); + + it('refuses a move into another account instead of sending a foreign id', async () => { + const { client } = makeClient({}); + await expect(client.updateFileNode('grp:n1', { parentId: null })).rejects.toThrow('between accounts'); + }); + + it('copies a folder with its whole subtree', async () => { + const { client, calls } = makeClient({ + me: [ + { id: 'd1', name: 'Docs', parentId: null, blobId: null }, + { id: 'f1', name: 'a.txt', parentId: 'd1', blobId: 'b1', type: 'text/plain', size: 3 }, + { id: 'd2', name: 'Sub', parentId: 'd1', blobId: null }, + { id: 'f2', name: 'b.txt', parentId: 'd2', blobId: 'b2', type: 'text/plain', size: 4 }, + ], + }); + await client.copyFileNode('d1', 'Docs (copy)', null); + + const creates = sets(calls).map(a => (a.create as { n0: Record }).n0); + expect(creates).toEqual([ + { name: 'Docs (copy)' }, + { name: 'a.txt', parentId: 'new1', type: 'text/plain', blobId: 'b1', size: 3 }, + { name: 'Sub', parentId: 'new1' }, + { name: 'b.txt', parentId: 'new3', type: 'text/plain', blobId: 'b2', size: 4 }, + ]); + expect(calls.some(([m]) => m === 'Blob/copy')).toBe(false); + }); + + it('copies file content across accounts with Blob/copy', async () => { + const { client, calls } = makeClient({ + grp: [{ id: 'f1', name: 'shared.pdf', parentId: null, blobId: 'b1', type: 'application/pdf', size: 9 }], + }); + const copy = await client.copyFileNode('grp:f1', 'shared.pdf', null); + + expect(calls.find(([m]) => m === 'Blob/copy')?.[1]).toEqual({ fromAccountId: 'grp', accountId: 'me', blobIds: ['b1'] }); + expect(sets(calls)[0]).toMatchObject({ accountId: 'me', create: { n0: { blobId: 'b1-copy' } } }); + expect(copy.id).toBe('new1'); + }); +}); diff --git a/lib/jmap/client.ts b/lib/jmap/client.ts index 9faa9c883..828d75f14 100644 --- a/lib/jmap/client.ts +++ b/lib/jmap/client.ts @@ -569,6 +569,12 @@ export function sendMethodErrors( return { failure, filing }; } +/** "report.pdf" -> "report (2).pdf", the way Stalwart's onExists "rename" names copies. */ +function numberedFileName(name: string, n: number): string { + const dot = name.lastIndexOf('.'); + return dot > 0 ? `${name.slice(0, dot)} (${n})${name.slice(dot)}` : `${name} (${n})`; +} + /** Throw on a method error of the query that a search request starts with. */ function assertQuerySucceeded(response: JMAPResponse, what: string): void { const [name, result] = response.methodResponses?.[0] ?? []; @@ -7440,26 +7446,105 @@ export class JMAPClient implements IJMAPClient { rights: FileNodeRights | null, targetAccountId?: string, ): Promise { - const accountId = targetAccountId || this.getFilesAccountId(); + const node = targetAccountId + ? { accountId: targetAccountId, id: fileNodeId } + : this.resolveFileNodeId(fileNodeId); + const accountId = node.accountId; + const rawId = node.id as string; const response = await this.request([ ["FileNode/set", { accountId, - update: { [fileNodeId]: { [`shareWith/${principalId}`]: rights } }, + update: { [rawId]: { [`shareWith/${principalId}`]: rights } }, }, "0"], ], this.fileUsing()); const result = response.methodResponses?.[0]?.[1]; - if (result?.notUpdated?.[fileNodeId]) { - const err = result.notUpdated[fileNodeId]; + if (result?.notUpdated?.[rawId]) { + const err = result.notUpdated[rawId]; throw new Error(err.description || "Failed to update file share"); } - if (!result?.updated || !(fileNodeId in result.updated)) { + if (!result?.updated || !(rawId in result.updated)) { throw new Error("Server did not confirm the share update"); } } + /** + * The account and bare id behind a FileNode id. Nodes of shared drives are + * listed as ":" (listAllFileNodesAcrossAccounts; JMAP ids + * never contain ':'), and a write must go to that account with the bare id. + * `null` is the primary account's root. + */ + private resolveFileNodeId(id: string | null): { accountId: string; id: string | null } { + const primary = this.getFilesAccountId(); + if (id === null) return { accountId: primary, id: null }; + const sep = id.indexOf(':'); + if (sep > 0) { + const accountId = id.slice(0, sep); + if (accountId !== primary && this.getFilesCapableAccountIds().includes(accountId)) { + return { accountId, id: id.slice(sep + 1) }; + } + } + return { accountId: primary, id }; + } + + /** Namespace a node created in `accountId` the way the listings do. */ + private namespaceFileNode(node: FileNode, accountId: string): FileNode { + if (accountId === this.getFilesAccountId()) return node; + return { + ...node, + id: `${accountId}:${node.id}`, + parentId: node.parentId == null ? null : `${accountId}:${node.parentId}`, + }; + } + + /** + * Resolve a parent for a node that lives in `accountId`. Folders only hold + * nodes of their own account, so a parent in another account is refused + * instead of being sent (the server would read it as a local id). + */ + private parentInAccount(parentId: string | null, accountId: string): string | null { + const parent = this.resolveFileNodeId(parentId); + if (parent.accountId !== accountId) { + throw new Error('Files cannot be moved between accounts'); + } + return parent.id; + } + + /** + * FileNode/set `create` of one node. `onExists: "rename"` (Stalwart 0.16.6+, + * ignored before) turns a name clash into "name (2)" instead of a failure. + */ + private async createFileNodeIn( + accountId: string, + props: Record, + what: string, + ): Promise { + const baseName = String(props.name ?? ''); + for (let attempt = 1; ; attempt++) { + const name = attempt === 1 ? baseName : numberedFileName(baseName, attempt); + const response = await this.request( + [["FileNode/set", { accountId, onExists: "rename", create: { n0: { ...props, name } } }, "fns0"]], + this.fileUsing(), + ); + const result = response.methodResponses?.[0]; + if (!result || result[0] === "error") { + throw new Error(result?.[1]?.description || "FileNode/set create failed"); + } + const created = result[1].created?.n0; + if (created) { + // The create response carries only server-set properties; keep the + // ones sent so callers can rely on name/parentId/type. + return withDecodedName({ ...props, name, ...created } as FileNode); + } + const err = result[1].notCreated?.n0; + // Servers before 0.16.6 ignore onExists and refuse the clash instead. + if (attempt < 20 && /already exists/i.test(err?.description ?? '')) continue; + throw new Error(err?.description || `Failed to create ${what}`); + } + } + async createFileDirectory(name: string, parentId: string | null): Promise { - const accountId = this.getFilesAccountId(); + const parent = this.resolveFileNodeId(parentId); // A FileNode is a folder (container) only when it has no content - i.e. no // blobId, type or size, so the server stores it with `file == null`. Sending @@ -7467,71 +7552,39 @@ export class JMAPClient implements IJMAPClient { // 0-byte FILE that nothing can ever be parented under, which is what caused // "Parent ID does not exist or is not a folder" during the #379 migration. const dirProps: Record = { name }; - if (parentId !== null) { - dirProps.parentId = parentId; + if (parent.id !== null) { + dirProps.parentId = parent.id; } - const response = await this.request( - [["FileNode/set", { - accountId, - create: { - dir0: dirProps, - }, - }, "fns0"]], - this.fileUsing(), - ); - - const result = response.methodResponses?.[0]; - if (!result || result[0] === "error") { - throw new Error(result?.[1]?.description || "FileNode/set create failed"); - } - const created = result[1].created?.dir0; - if (!created) { - const err = result[1].notCreated?.dir0; - throw new Error(err?.description || "Failed to create directory"); - } - return created as FileNode; + const created = await this.createFileNodeIn(parent.accountId, dirProps, 'directory'); + return this.namespaceFileNode(created, parent.accountId); } async createFileNode(name: string, blobId: string, type: string, size: number, parentId: string | null): Promise { - const accountId = this.getFilesAccountId(); + const parent = this.resolveFileNodeId(parentId); //fall back for long MIME types const safeType = type.length > 30 ? 'application/octet-stream' : type; const fileProps: Record = { name, type: safeType, blobId, size }; - if (parentId !== null) { - fileProps.parentId = parentId; + if (parent.id !== null) { + fileProps.parentId = parent.id; } - const response = await this.request( - [["FileNode/set", { - accountId, - create: { - file0: fileProps, - }, - }, "fns0"]], - this.fileUsing(), - ); - - const result = response.methodResponses?.[0]; - if (!result || result[0] === "error") { - throw new Error(result?.[1]?.description || "FileNode/set create failed"); - } - const created = result[1].created?.file0; - if (!created) { - const err = result[1].notCreated?.file0; - throw new Error(err?.description || "Failed to create file node"); - } - return created as FileNode; + const created = await this.createFileNodeIn(parent.accountId, fileProps, 'file node'); + return this.namespaceFileNode(created, parent.accountId); } async updateFileNode(id: string, updates: Partial>): Promise { - const accountId = this.getFilesAccountId(); + const node = this.resolveFileNodeId(id); + const patch: Record = { ...updates }; + if ('parentId' in updates) { + patch.parentId = this.parentInAccount(updates.parentId ?? null, node.accountId); + } const response = await this.request( [["FileNode/set", { - accountId, - update: { [id]: updates }, + accountId: node.accountId, + update: { [node.id as string]: patch }, }, "fns0"]], this.fileUsing(), ); @@ -7540,11 +7593,23 @@ export class JMAPClient implements IJMAPClient { if (!result || result[0] === "error") { throw new Error(result?.[1]?.description || "FileNode/set update failed"); } - if (result[1].notUpdated?.[id]) { - throw new Error(result[1].notUpdated[id].description || "Failed to update file node"); + if (result[1].notUpdated?.[node.id as string]) { + throw new Error(result[1].notUpdated[node.id as string].description || "Failed to update file node"); } } + /** Group FileNode ids by the account that owns them. */ + private groupFileNodeIds(entries: Array<[string, T]>): Map> { + const groups = new Map>(); + for (const [id, value] of entries) { + const node = this.resolveFileNodeId(id); + const list = groups.get(node.accountId) ?? []; + list.push({ id, rawId: node.id as string, value }); + groups.set(node.accountId, list); + } + return groups; + } + /** * Update many FileNodes in a single FileNode/set call. Returns the ids that * were updated and a map of id -> error for any the server rejected. Never @@ -7553,106 +7618,136 @@ export class JMAPClient implements IJMAPClient { async updateFileNodes(updates: Record>>): Promise<{ updated: string[]; notUpdated: Record }> { const entries = Object.entries(updates); if (entries.length === 0) return { updated: [], notUpdated: {} }; - const accountId = this.getFilesAccountId(); const updated: string[] = []; const notUpdated: Record = {}; - for (const batch of batched(entries, this.getMaxObjectsInSet())) { - const response = await this.request( - [["FileNode/set", { accountId, update: Object.fromEntries(batch) }, "fns0"]], - this.fileUsing(), - ); + for (const [accountId, nodes] of this.groupFileNodeIds(entries)) { + for (const batch of batched(nodes, this.getMaxObjectsInSet())) { + const byRawId = new Map(batch.map(n => [n.rawId, n.id])); + const update: Record> = {}; + for (const n of batch) { + const patch: Record = { ...n.value }; + if ('parentId' in n.value) { + try { + patch.parentId = this.parentInAccount(n.value.parentId ?? null, accountId); + } catch (error) { + notUpdated[n.id] = error instanceof Error ? error.message : 'not updated'; + continue; + } + } + update[n.rawId] = patch; + } + if (Object.keys(update).length === 0) continue; - const result = response.methodResponses?.[0]; - if (!result || result[0] === "error") { - throw new Error(result?.[1]?.description || "FileNode/set update failed"); - } + const response = await this.request( + [["FileNode/set", { accountId, update }, "fns0"]], + this.fileUsing(), + ); - const updatedMap: Record = result[1].updated || {}; - const notUpdatedMap: Record = result[1].notUpdated || {}; - for (const id of Object.keys(notUpdatedMap)) { - notUpdated[id] = notUpdatedMap[id]?.description || 'not updated'; + const result = response.methodResponses?.[0]; + if (!result || result[0] === "error") { + throw new Error(result?.[1]?.description || "FileNode/set update failed"); + } + + const updatedMap: Record = result[1].updated || {}; + const notUpdatedMap: Record = result[1].notUpdated || {}; + for (const rawId of Object.keys(notUpdatedMap)) { + notUpdated[byRawId.get(rawId) ?? rawId] = notUpdatedMap[rawId]?.description || 'not updated'; + } + // Servers may omit the `updated` map; treat anything not rejected as updated. + const done = Object.keys(updatedMap).length > 0 + ? Object.keys(updatedMap).map(rawId => byRawId.get(rawId) ?? rawId) + : Object.keys(update).map(rawId => byRawId.get(rawId) as string).filter(id => !(id in notUpdated)); + updated.push(...done); } - // Servers may omit the `updated` map; treat anything not rejected as updated. - updated.push(...(Object.keys(updatedMap).length > 0 - ? Object.keys(updatedMap) - : batch.map(([id]) => id).filter(id => !(id in notUpdated)))); } return { updated, notUpdated }; } async destroyFileNodes(ids: string[]): Promise<{ destroyed: string[]; notDestroyed: string[] }> { - const accountId = this.getFilesAccountId(); const destroyed: string[] = []; - for (const batch of batched(ids, this.getMaxObjectsInSet())) { - const response = await this.request( - [["FileNode/set", { - accountId, - destroy: batch, - onDestroyRemoveChildren: true, - }, "fns0"]], - this.fileUsing(), - ); + for (const [accountId, nodes] of this.groupFileNodeIds(ids.map(id => [id, null] as [string, null]))) { + for (const batch of batched(nodes, this.getMaxObjectsInSet())) { + const byRawId = new Map(batch.map(n => [n.rawId, n.id])); + const response = await this.request( + [["FileNode/set", { + accountId, + destroy: batch.map(n => n.rawId), + onDestroyRemoveChildren: true, + }, "fns0"]], + this.fileUsing(), + ); - const result = response.methodResponses?.[0]; - if (!result || result[0] === "error") { - throw new Error(result?.[1]?.description || "FileNode/set destroy failed"); + const result = response.methodResponses?.[0]; + if (!result || result[0] === "error") { + throw new Error(result?.[1]?.description || "FileNode/set destroy failed"); + } + + const notDestroyedMap: Record = result[1].notDestroyed || {}; + const notDestroyedIds = Object.keys(notDestroyedMap); + + if (notDestroyedIds.length > 0) { + const firstError = notDestroyedMap[notDestroyedIds[0]]; + throw new Error(firstError?.description || `Failed to delete ${notDestroyedIds.length} file(s)`); + } + + destroyed.push(...((result[1].destroyed || []) as string[]).map(rawId => byRawId.get(rawId) ?? rawId)); } - - const notDestroyedMap: Record = result[1].notDestroyed || {}; - const notDestroyedIds = Object.keys(notDestroyedMap); - - if (notDestroyedIds.length > 0) { - const firstError = notDestroyedMap[notDestroyedIds[0]]; - throw new Error(firstError?.description || `Failed to delete ${notDestroyedIds.length} file(s)`); - } - - destroyed.push(...(result[1].destroyed || [])); } return { destroyed, notDestroyed: [] }; } + /** + * Copy a file, or a folder with everything in it, into `parentId` (which + * may be in another account). A folder node carries no content, so creating + * one "copy" node would only make an empty folder: the subtree is recreated + * level by level. Across accounts the file contents go through Blob/copy. + */ async copyFileNode(id: string, newName: string, parentId: string | null): Promise { - // Copy: get original, upload blob reference, create new node - const nodes = await this.getFileNodes([id]); - if (nodes.length === 0) throw new Error('File node not found'); - const original = nodes[0]; + const source = this.resolveFileNodeId(id); + const target = this.resolveFileNodeId(parentId); - const accountId = this.getFilesAccountId(); - const createProps: Record = { - name: newName, - type: original.type, - blobId: original.blobId, - size: original.size, + const tree = await this.fetchAllFileNodes(source.accountId); + const original = tree.find(n => n.id === source.id); + if (!original) throw new Error('File node not found'); + const isFolderNode = (n: FileNode) => n.blobId == null; + + const copyBlob = async (blobId: string): Promise => { + if (source.accountId === target.accountId) return blobId; + const response = await this.request([ + ["Blob/copy", { fromAccountId: source.accountId, accountId: target.accountId, blobIds: [blobId] }, "bc0"], + ], this.fileUsing()); + const result = response.methodResponses?.[0]; + const copied = result?.[0] === "Blob/copy" ? result[1].copied?.[blobId] : undefined; + if (!copied) { + throw new Error(result?.[1]?.notCopied?.[blobId]?.description || result?.[1]?.description || "Failed to copy file content"); + } + return copied as string; }; - if (parentId !== null) { - createProps.parentId = parentId; - } - const response = await this.request( - [["FileNode/set", { - accountId, - create: { - copy0: createProps, - }, - }, "fns0"]], - this.fileUsing(), - ); + const copyInto = async (node: FileNode, name: string, parent: string | null): Promise => { + const props: Record = { name }; + if (parent !== null) props.parentId = parent; + if (!isFolderNode(node)) { + props.type = node.type; + props.blobId = await copyBlob(node.blobId as string); + props.size = node.size; + } + const created = await this.createFileNodeIn(target.accountId, props, 'copy'); + if (isFolderNode(node)) { + for (const child of tree.filter(n => n.parentId === node.id)) { + await copyInto(child, child.name, created.id); + } + } + return created; + }; - const result = response.methodResponses?.[0]; - if (!result || result[0] === "error") { - throw new Error(result?.[1]?.description || "FileNode copy failed"); - } - const created = result[1].created?.copy0; - if (!created) { - const err = result[1].notCreated?.copy0; - throw new Error(err?.description || "Failed to copy file node"); - } - return created as FileNode; + const created = await copyInto(withDecodedName(original), newName, target.id); + return this.namespaceFileNode(created, target.accountId); } async downloadBlob(blobId: string, name?: string, type?: string, accountId?: string): Promise {