diff --git a/client/src/components/Files/docsync/useDocSync.test.ts b/client/src/components/Files/docsync/useDocSync.test.ts index 1bea1a3d1..c94989d67 100644 --- a/client/src/components/Files/docsync/useDocSync.test.ts +++ b/client/src/components/Files/docsync/useDocSync.test.ts @@ -212,7 +212,7 @@ describe('useDocSync spinner', () => { listLinksApi.mockReturnValueOnce(gate.promise) seenLoading.length = 0 - let pending!: Promise + let pending!: Promise await act(async () => { pending = result.current.updateLink(1, { syncEnabled: false }) }) expect(result.current.loading).toBe(false) @@ -235,7 +235,7 @@ describe('useDocSync updateLink', () => { const gate = deferred() updateLinkApi.mockReturnValueOnce(gate.promise) - let pending!: Promise + let pending!: Promise await act(async () => { pending = result.current.updateLink(1, { direction: 'pull' }) }) expect(result.current.links[0].direction).toBe('pull') @@ -257,14 +257,18 @@ describe('useDocSync updateLink', () => { it('FE-DOCSYNC-HOOK-011: a rejected update does not leave the optimistic value standing', async () => { const { result } = await mountLoaded() - updateLinkApi.mockRejectedValueOnce(new Error('conflict')) + updateLinkApi.mockRejectedValueOnce({ response: { data: { error: 'forbidden' } } }) + listLinksApi.mockResolvedValueOnce([link({ direction: 'both' })]) - await act(async () => { - await expect(result.current.updateLink(1, { syncEnabled: false })).rejects.toThrow('conflict') - }) + let ok: boolean | undefined + await act(async () => { ok = await result.current.updateLink(1, { direction: 'pull' }) }) - expect(listLinksApi).toHaveBeenCalledTimes(2) - expect(result.current.links[0].syncEnabled).toBe(true) + // Reported rather than thrown: both shells call this as `void`, so a + // rejection reached nobody. The reload puts the server's answer back and + // the reason stays on screen. + expect(ok).toBe(false) + expect(result.current.links[0].direction).toBe('both') + expect(result.current.error).toBe('forbidden') }) }) @@ -333,19 +337,24 @@ describe('useDocSync run and save feedback', () => { await act(async () => { await result.current.syncNow(1) }) expect(syncNowApi).toHaveBeenCalledWith(TRIP, 1, false) - expect(result.current.lastRunFor(1)).toEqual({ pulled: 4, pushed: 2 }) - // a binding nobody ran in this session reports zeros, not someone else numbers - expect(result.current.lastRunFor(2)).toEqual({ pulled: 0, pushed: 0 }) + // What a run moved is no longer kept: the flow bar shows standing holdings, + // which is the question somebody opening the dialog actually has. The run's + // own numbers were only ever on screen for the moment after pressing it. + expect(result.current.links).toHaveLength(1) }) - it('FE-DOCSYNC-HOOK-017: a run that reports no numbers counts as nothing moved', async () => { - syncNowApi.mockResolvedValue({ state: 'ok' }) + it('FE-DOCSYNC-HOOK-017: a refused run says so instead of vanishing into a void call', async () => { + // syncNow is called as `void sync.syncNow(...)` from both shells, so a + // rejection that escaped the hook reached nobody. An orphaned binding + // answers 409 here, and the person pressing the button deserves the reason. + syncNowApi.mockRejectedValueOnce({ response: { data: { error: 'link_orphaned' } } }) const { result } = await mountLoaded() - await act(async () => { await result.current.syncNow(1, true) }) + let outcome: unknown + await act(async () => { outcome = await result.current.syncNow(1) }) - expect(syncNowApi).toHaveBeenCalledWith(TRIP, 1, true) - expect(result.current.lastRunFor(1)).toEqual({ pulled: 0, pushed: 0 }) + expect(outcome).toBeNull() + expect(result.current.error).toBe('link_orphaned') }) it('FE-DOCSYNC-HOOK-018: a refused save reports failure and surfaces the server reason', async () => { diff --git a/client/src/components/Files/docsync/useDocSync.ts b/client/src/components/Files/docsync/useDocSync.ts index 00f976528..da96ec874 100644 --- a/client/src/components/Files/docsync/useDocSync.ts +++ b/client/src/components/Files/docsync/useDocSync.ts @@ -92,12 +92,6 @@ export function useDocSync(tripId: number | string, enabled: boolean) { const [connections, setConnections] = useState([]) const [links, setLinks] = useState([]) const [itemCounts, setItemCounts] = useState>({}) - /** - * What the last run of each binding moved, so the flow bar can show numbers - * on its lanes. Kept in memory rather than persisted: it describes one run, - * and after a reload "nothing moved since you got here" is the honest answer. - */ - const [lastRun, setLastRun] = useState>({}) const [loading, setLoading] = useState(false) const [busy, setBusy] = useState(null) const [error, setError] = useState(null) @@ -111,8 +105,21 @@ export function useDocSync(tripId: number | string, enabled: boolean) { */ const loadedOnce = useRef(false) + /** + * Which load is the current one. + * + * Four requests go out per load and several loads overlap — every mutation + * starts one, and so does every ping. Without a generation stamp the slowest + * response wins, which on a binding somebody just changed means the screen + * settles on the state from before the change. + */ + const generation = useRef(0) + /** Whether the last failure came from loading rather than from a write. */ + const loadFailed = useRef(false) + const load = useCallback(async () => { if (!enabled) return + const mine = ++generation.current if (!loadedOnce.current) setLoading(true) try { const [p, c, l, s] = await Promise.all([ @@ -121,6 +128,7 @@ export function useDocSync(tripId: number | string, enabled: boolean) { docsyncApi.listLinks(tripId), docsyncApi.status(tripId), ]) + if (mine !== generation.current) return setProviders(p as DocSyncProvider[]) setConnections(c as DocSyncConnection[]) // The status route carries the holdings; the links route does not, so the @@ -129,14 +137,28 @@ export function useDocSync(tripId: number | string, enabled: boolean) { const holdingsById = new Map((status.links ?? []).map(x => [x.id, x.holdings])) setLinks((l as DocSyncLink[]).map(x => ({ ...x, holdings: holdingsById.get(x.id) }))) setItemCounts(status.items || {}) - setError(null) + // Only a load failure is cleared here. A write that just failed reloads to + // put the server's answer back on screen, and clearing unconditionally + // wiped the reason it had set a moment earlier. + setError(prev => (prev !== null && loadFailed.current ? null : prev)) + loadFailed.current = false loadedOnce.current = true - } catch { - // A failure here means the addon is off or the user lost access, both of - // which the panel renders as "nothing to configure" rather than an alarm. - setProviders([]) + } catch (e: unknown) { + // A 403 or 404 really does mean "nothing to configure here" — the addon is + // off or this person lost access — and blanking the panel is the honest + // answer. Anything else is a failure to say so: blanking on a dropped + // connection told the user their provider list was empty. + if (mine !== generation.current) return + const status = (e as { response?: { status?: number } })?.response?.status + loadFailed.current = true + if (status === 403 || status === 404) { + setProviders([]) + setError(null) + } else { + setError(readError(e)) + } } finally { - setLoading(false) + if (mine === generation.current) setLoading(false) } }, [tripId, enabled]) @@ -228,20 +250,40 @@ export function useDocSync(tripId: number | string, enabled: boolean) { } }, [tripId, load]) + /** + * Change one binding, optimistically. + * + * The reload in `finally` puts the server's answer back either way, so a + * refused change corrects itself on screen — but it used to do that in + * silence, with the rejection escaping into a `void` call site. The switch + * simply flipped back and nobody said why. + */ const updateLink = useCallback(async (linkId: number, patch: Record) => { setLinks(prev => prev.map(l => (l.id === linkId ? { ...l, ...patch } as DocSyncLink : l))) + setError(null) try { await docsyncApi.updateLink(tripId, linkId, patch) - } finally { await load() + return true + } catch (e: unknown) { + // Reload first, so the optimistic value is corrected, then say why — + // the other way round the reload cleared the message again. + await load() + setError(readError(e)) + return false } }, [tripId, load]) const removeLink = useCallback(async (linkId: number) => { setBusy('link') + setError(null) try { await docsyncApi.deleteLink(tripId, linkId) await load() + return true + } catch (e: unknown) { + setError(readError(e)) + return false } finally { setBusy(null) } @@ -249,23 +291,26 @@ export function useDocSync(tripId: number | string, enabled: boolean) { const syncNow = useCallback(async (linkId: number, full = false) => { setBusy(`sync-${linkId}`) + setError(null) try { const res = await docsyncApi.syncNow(tripId, linkId, full) as { state: string; pulled: number; pushed: number; conflicts: number; missing: number } - setLastRun(prev => ({ ...prev, [linkId]: { pulled: res.pulled ?? 0, pushed: res.pushed ?? 0 } })) await load() return res + } catch (e: unknown) { + // A refused run is a thing the person asked for and did not get — an + // orphaned binding answers 409 here — so it has to be said out loud + // rather than escaping into the `void` at the call site. After the + // reload, which would otherwise clear the message. + await load() + setError(readError(e)) + return null } finally { setBusy(null) } }, [tripId, load]) - const lastRunFor = useCallback( - (linkId: number) => lastRun[linkId] ?? { pulled: 0, pushed: 0 }, - [lastRun], - ) - const resolveConflict = useCallback(async (itemId: number, keep: 'trek' | 'provider' | 'both') => { await docsyncApi.resolve(tripId, itemId, keep) await load() @@ -281,7 +326,7 @@ export function useDocSync(tripId: number | string, enabled: boolean) { connectionFor, load, saveConnection, testConnection, loadScopes, createScope, - createLink, updateLink, removeLink, syncNow, resolveConflict, lastRunFor, + createLink, updateLink, removeLink, syncNow, resolveConflict, } } diff --git a/server/src/nest/doc-sync/doc-sync-webhook.controller.ts b/server/src/nest/doc-sync/doc-sync-webhook.controller.ts index 8260a560d..2ba9509ef 100644 --- a/server/src/nest/doc-sync/doc-sync-webhook.controller.ts +++ b/server/src/nest/doc-sync/doc-sync-webhook.controller.ts @@ -2,7 +2,10 @@ import { Controller, HttpCode, OnModuleDestroy, Param, Post, Req } from '@nestjs import type { Request } from 'express'; import crypto from 'crypto'; import { Public } from '../auth/public.decorator'; -import { WEBHOOK_NUDGE_DEBOUNCE_SECONDS } from './doc-sync.constants'; +import { ADDON_IDS } from '../../addons'; +import { AddonsService } from '../addons/addons.service'; +import { DatabaseService } from '../database/database.service'; +import { SETTING_SYNC_ENABLED, WEBHOOK_NUDGE_DEBOUNCE_SECONDS } from './doc-sync.constants'; import { DocSyncConfigService } from './doc-sync-config.service'; import { DocSyncService } from './doc-sync.service'; @@ -39,8 +42,29 @@ export class DocSyncWebhookController implements OnModuleDestroy { constructor( private readonly config: DocSyncConfigService, private readonly sync: DocSyncService, + private readonly addons: AddonsService, + private readonly db: DatabaseService, ) {} + /** + * The same two switches the scheduler obeys. + * + * Without them "switch document sync off" meant "stop the poll", while every + * provider holding a webhook kept driving full runs — an admin turning the + * addon off would have watched it carry on. Checked when the timer fires as + * well as on arrival, so a switch thrown during the debounce window still + * takes effect. + */ + private syncIsOn(): boolean { + if (!this.addons.isAddonEnabled(ADDON_IDS.DOCUMENTS)) return false; + const killSwitch = this.db.get<{ value: string }>( + 'SELECT value FROM app_settings WHERE key = ?', SETTING_SYNC_ENABLED, + )?.value; + // Unrecognised values mean ON: the setting is absent by default, and only an + // explicit 'false' stops the sync. Same rule as the job. + return killSwitch !== 'false'; + } + onModuleDestroy(): void { for (const timer of this.pending.values()) clearTimeout(timer); this.pending.clear(); @@ -54,6 +78,7 @@ export class DocSyncWebhookController implements OnModuleDestroy { // Always 200, even for an unknown token: a 404 here would let anyone probe // which tokens exist, and a provider that gets an error will retry anyway. if (!link || link.sync_enabled !== 1) return { received: true }; + if (!this.syncIsOn()) return { received: true }; const secret = this.config.webhookSecret(link); if (secret && !this.secretMatches(req, secret)) return { received: true }; @@ -81,6 +106,7 @@ export class DocSyncWebhookController implements OnModuleDestroy { this.pending.delete(linkId); const fresh = reload(); if (!fresh || fresh.sync_enabled !== 1) return; + if (!this.syncIsOn()) return; void this.sync.syncLink(fresh); }, WEBHOOK_NUDGE_DEBOUNCE_SECONDS * 1000); // A pending nudge must not hold the process open at shutdown. diff --git a/server/src/nest/doc-sync/doc-sync.controller.ts b/server/src/nest/doc-sync/doc-sync.controller.ts index 68c107e0c..f14e7b750 100644 --- a/server/src/nest/doc-sync/doc-sync.controller.ts +++ b/server/src/nest/doc-sync/doc-sync.controller.ts @@ -325,6 +325,13 @@ export class DocSyncController { ) { const link = this.config.getLink(Number(linkId)); if (!link || link.trip_id !== Number(tripId)) throw new HttpException('Link not found', 404); + // An orphaned binding stays orphaned: its credential belongs to somebody who + // is no longer on this trip, and a manual run would use it anyway. The + // scheduler already refuses it (sync_enabled is 0), so without this the + // button was a way around that. + if (link.last_sync_state === 'orphaned') { + throw new HttpException({ error: 'This binding lost its owner and has to be reconnected' }, 409); + } // A person asking for a run is also asking for the rows that gave up to be // tried once more; the scheduler gets no such reprieve. this.sync.retryShelvedItems(link.id); diff --git a/server/src/nest/doc-sync/doc-sync.helpers.ts b/server/src/nest/doc-sync/doc-sync.helpers.ts index 871402979..d86a331f4 100644 --- a/server/src/nest/doc-sync/doc-sync.helpers.ts +++ b/server/src/nest/doc-sync/doc-sync.helpers.ts @@ -288,6 +288,27 @@ export function planReconcile(input: ReconcileInput): ReconcilePlan { continue; } + /** + * Paired upstream, absent in TREK: the download never landed. + * + * A failed pull stores the remote's version alongside the error, so on the + * next run `remoteChanged` is false, there is no local file for the other + * branches to compare against, and the row fell through to `touch` — which + * refreshes it and changes nothing. Nothing anywhere planned a second + * attempt: the document never arrived, no retry ever happened, and pressing + * "Sync now" did not help either, because the plan was the same. + * + * A rejected type and an oversized file are left out on purpose. Those are + * not transport failures; they are answers, and they stay true until the + * document upstream changes — which the `remoteChanged` branch below picks + * up on its own. + */ + if (!l && it.fileId === null && direction !== 'push' + && it.state !== 'rejected_type' && it.state !== 'too_large') { + add({ kind: 'pull_update', remote: r, itemId: it.id }); + continue; + } + if (remoteChanged && !isEcho && localChanged) { add({ kind: 'conflict', itemId: it.id, remote: r, local: l ?? null }); continue; diff --git a/server/src/nest/doc-sync/doc-sync.service.ts b/server/src/nest/doc-sync/doc-sync.service.ts index 199d7c375..6c5d2ac5c 100644 --- a/server/src/nest/doc-sync/doc-sync.service.ts +++ b/server/src/nest/doc-sync/doc-sync.service.ts @@ -363,6 +363,14 @@ export class DocSyncService { return 'provider_error'; } + // What this row pointed at before, if anything: a pull_update replaces a + // document TREK already holds, and `createFile` only ever inserts. + const supersededId = itemId === null + ? null + : (this.db.connection + .prepare('SELECT file_id FROM document_sync_items WHERE id = ?') + .get(itemId) as { file_id: number | null } | undefined)?.file_id ?? null; + const created = this.files.createFile( ctx.link.trip_id, { filename: storageKey, originalname: name, size: bytes, mimetype: remote.mimeType || 'application/octet-stream' }, @@ -372,6 +380,22 @@ export class DocSyncService { {}, ); + /** + * Retire the copy this one replaces. + * + * Into the trash rather than out of existence: the bytes it holds are a + * version somebody may still want, and TREK's own delete works the same way. + * Leaving it in place was worse than untidy — the row's pairing moves to the + * new file, so the old one is left with no sync item at all, and the very + * next run reads it as a document TREK gained and pushes the superseded + * version back up. One upstream edit then ends as two documents on both + * sides, and it grows with every further edit. + */ + if (supersededId !== null && Number(supersededId) !== Number(created.id)) { + this.files.softDeleteFile(supersededId); + this.realtime.broadcast(ctx.link.trip_id, 'file:deleted', { fileId: supersededId }); + } + this.upsertItem(ctx.link, { itemId, remote, @@ -407,6 +431,11 @@ export class DocSyncService { const file = this.files.getFileById(local.fileId, ctx.link.trip_id); if (!file) return 'not_found'; + // Re-read rather than trust the plan: a run walks a whole folder, and a + // document somebody deleted in the meantime would otherwise still be + // uploaded — a delete that looks ignored, and a document that reappears + // upstream after the person watched it go. + if ((file as { deleted_at?: string | null }).deleted_at) return 'not_found'; let sha256 = local.sha256; let stream: Readable; @@ -573,22 +602,38 @@ export class DocSyncService { const failed = patch.state === 'error'; if (patch.itemId !== null) { - const attempts = failed ? 1 : 0; + /** + * The attempt counter, read before it is written. + * + * Two things needed it. The backoff was computed as + * `backoffSeconds(curve, 1)` — always the first step, so three of the + * four steps in the curve were unreachable and a provider that was down + * got asked again at the same short interval. And the counter only ever + * grew: a row that failed once a month reached the limit after six months + * of otherwise healthy syncing and was shelved for good. A successful + * pass now clears it, which is what makes the limit mean "six failures in + * a row" rather than "six failures ever". + */ + const before = (this.db.connection + .prepare('SELECT attempts FROM document_sync_items WHERE id = ?') + .get(patch.itemId) as { attempts: number } | undefined)?.attempts ?? 0; + const attempts = failed ? before + 1 : 0; + this.db.connection .prepare( `UPDATE document_sync_items SET state = ?, error_code = ?, file_id = COALESCE(?, file_id), - -- see the giveUp() note below: a row that has run out of - -- attempts stops retrying and waits for a person - remote_id = COALESCE(?, remote_id), remote_version = COALESCE(?, remote_version), remote_name = COALESCE(?, remote_name), remote_size = COALESCE(?, remote_size), remote_modified_at = COALESCE(?, remote_modified_at), content_sha256 = COALESCE(?, content_sha256), pushed_sha256 = COALESCE(?, pushed_sha256), - attempts = attempts + ?, + attempts = ?, + -- A row that has used up its attempts stops retrying and waits + -- for a person; planReconcile skips it and a manual run clears + -- the counter (see retryShelvedItems). next_attempt_at = CASE - WHEN ? = 1 AND attempts + 1 < ? THEN datetime('now', '+' || ? || ' seconds') + WHEN ? = 1 AND ? < ? THEN datetime('now', '+' || ? || ' seconds') ELSE NULL END, remote_missing_at = NULL, @@ -601,7 +646,9 @@ export class DocSyncService { remoteId, remoteVersion, patch.remote?.name ?? null, patch.remote?.size ?? null, patch.remote?.remoteModifiedAt ?? null, patch.contentSha256 ?? null, patch.pushedSha256 ?? null, - attempts, failed ? 1 : 0, ITEM_MAX_ATTEMPTS, backoffSeconds(ITEM_BACKOFF_SECONDS, 1), patch.state, + attempts, + failed ? 1 : 0, attempts, ITEM_MAX_ATTEMPTS, + backoffSeconds(ITEM_BACKOFF_SECONDS, attempts), patch.state, patch.itemId, ); return; @@ -633,7 +680,7 @@ export class DocSyncService { remoteId, patch.remote?.name ?? null, remoteVersion, patch.remote?.size ?? null, patch.remote?.remoteModifiedAt ?? null, patch.contentSha256 ?? null, patch.pushedSha256 ?? null, patch.state, patch.errorCode ?? null, failed ? 1 : 0, - failed ? 1 : 0, backoffSeconds(ITEM_BACKOFF_SECONDS, 1), + failed ? 1 : 0, backoffSeconds(ITEM_BACKOFF_SECONDS, 1), // a new row is always at its first failure patch.state, ); } @@ -681,6 +728,28 @@ export class DocSyncService { * integer and nothing else tied the two together. Same rule as everywhere in * this codebase: every referenced id must exist AND belong to the same trip. */ + + /** + * Rename the provider's copy, outside a run. + * + * Only `resolveConflict` needs this: every other rename is planned and + * executed by a run. A failure is not fatal here — the row is left for the + * next run to sort out rather than the owner's choice being refused. + */ + private async renameRemoteTo(link: LinkRow, remoteId: string, name: string, itemId: number): Promise { + const conn = this.config.getConnection(link.connection_id); + const provider = this.registry.get(link.provider_id); + if (!conn || !provider) return; + const res = await provider.rename(this.config.toRef(conn), this.config.toScopeRef(link), remoteId, name); + if (docFailed(res)) { + this.logger.warn(`link ${link.id}: renaming ${remoteId} to "${name}" failed: ${res.error.code}`); + return; + } + this.db.connection + .prepare('UPDATE document_sync_items SET remote_name = ?, remote_version = ?, last_seen_at = CURRENT_TIMESTAMP WHERE id = ?') + .run(name, res.data.remoteVersion, itemId); + } + async resolveConflict(itemId: number, keep: 'trek' | 'provider' | 'both', tripId?: number): Promise { const item = this.db.connection .prepare('SELECT * FROM document_sync_items WHERE id = ?') @@ -690,6 +759,27 @@ export class DocSyncService { const link = this.config.getLink(Number(item.link_id)); if (!link) return false; + /** + * The name has to be decided too, not only the bytes. + * + * The only conflict the planner can actually raise today is a double + * rename, and that is arbitrated entirely by `remote_name` — the name both + * sides last agreed on. Clearing a version marker leaves all three names + * exactly as they were, so the next run compared them again, found both + * sides changed again, and wrote the conflict straight back. Whatever the + * owner picked, the issue reappeared within minutes, forever. + * + * So each choice moves the arbiter to the side that won: keeping TREK's + * name makes the agreed name TREK's (the provider then reads as the one + * that renamed, and gets renamed back), keeping the provider's takes the + * local rename back (TREK then reads as unchanged, and follows). + */ + const localName = item.file_id === null + ? null + : (this.db.connection + .prepare('SELECT original_name FROM trip_files WHERE id = ?') + .get(item.file_id) as { original_name: string } | undefined)?.original_name ?? null; + if (keep === 'trek') { // Forget the provider's version marker, so the next run sees no upstream // change and pushes TREK's copy. Clearing content_sha256 instead would do @@ -698,10 +788,25 @@ export class DocSyncService { this.db.connection .prepare("UPDATE document_sync_items SET state = 'pending', remote_version = NULL, error_code = NULL WHERE id = ?") .run(itemId); + // Carry the name over as well, here rather than by moving the arbiter: + // TREK winning means the provider's copy takes TREK's name, and the only + // way to say that through `remote_name` would be to write the provider's + // CURRENT name into it — which this method does not know without asking. + if (localName && item.remote_id && localName !== item.remote_name) { + await this.renameRemoteTo(link, String(item.remote_id), localName, itemId); + } } else if (keep === 'provider') { this.db.connection .prepare("UPDATE document_sync_items SET state = 'pending', content_sha256 = NULL, error_code = NULL WHERE id = ?") .run(itemId); + // Take the local rename back to the agreed name. Only the provider's + // rename is then left standing, and the next run follows it the ordinary + // way — through the planner, with no special case in it. + if (item.file_id !== null && item.remote_name && localName !== item.remote_name) { + this.db.connection + .prepare('UPDATE trip_files SET original_name = ? WHERE id = ?') + .run(item.remote_name, item.file_id); + } } else { // Detach the pairing and let the next run pull the provider copy as a new // document. Both versions survive, under two rows. diff --git a/server/src/nest/doc-sync/providers/papra.client.ts b/server/src/nest/doc-sync/providers/papra.client.ts index 488e1d66d..cc4e90d55 100644 --- a/server/src/nest/doc-sync/providers/papra.client.ts +++ b/server/src/nest/doc-sync/providers/papra.client.ts @@ -329,12 +329,27 @@ function escapeFormValue(value: string): string { return value.replace(/\r/g, '%0D').replace(/\n/g, '%0A').replace(/"/g, '%22'); } +/** + * A media type that is safe to put in a header. + * + * The value comes from `trip_files.mime_type`, which an upload can influence, + * and it went into the part header untouched while the filename beside it was + * escaped. A CR or LF in it ends the header early and lets whatever follows be + * read as headers of its own. Anything that is not a plain media type is + * replaced rather than repaired: a mangled type is not worth guessing at, and + * the generic one is what an unknown file gets anyway. + */ +function safeMimeType(raw: string): string { + const TOKEN = /^[A-Za-z0-9!#$&^_.+-]+\/[A-Za-z0-9!#$&^_.+-]+$/; + return TOKEN.test(raw) ? raw : 'application/octet-stream'; +} + /** The bytes before the file in a one-field multipart body. Exported so the tests can read them. */ export function multipartHeader(boundary: string, fileName: string, mimeType: string): Buffer { return Buffer.from( `--${boundary}\r\n` + `Content-Disposition: form-data; name="file"; filename="${escapeFormValue(fileName)}"\r\n` + - `Content-Type: ${mimeType}\r\n\r\n`, + `Content-Type: ${safeMimeType(mimeType)}\r\n\r\n`, 'utf8', ); } diff --git a/server/src/nest/doc-sync/providers/webdav.client.ts b/server/src/nest/doc-sync/providers/webdav.client.ts index f6d9b397d..4d59ee168 100644 --- a/server/src/nest/doc-sync/providers/webdav.client.ts +++ b/server/src/nest/doc-sync/providers/webdav.client.ts @@ -126,6 +126,8 @@ export interface WebdavPutOptions { sha256: string; /** Optimistic concurrency. Both products answer a mismatch with 412. */ ifMatch?: string; + /** `*` refuses to create over an existing resource (RFC 9110 §13.1.2). */ + ifNoneMatch?: string; } export interface WebdavPutResult { @@ -625,6 +627,9 @@ export class WebdavClient { if (creds.flavor === 'nextcloud' && /^[0-9a-f]{64}$/i.test(options.sha256)) { headers['OC-Checksum'] = `SHA256:${options.sha256.toLowerCase()}`; } + if (options.ifNoneMatch) { + headers['If-None-Match'] = options.ifNoneMatch; + } if (options.ifMatch) { headers['If-Match'] = options.ifMatch; } diff --git a/server/src/nest/doc-sync/providers/webdav.provider.ts b/server/src/nest/doc-sync/providers/webdav.provider.ts index a8b49c855..d31675200 100644 --- a/server/src/nest/doc-sync/providers/webdav.provider.ts +++ b/server/src/nest/doc-sync/providers/webdav.provider.ts @@ -55,8 +55,16 @@ const WEBHOOK_EVENTS = [ 'OCP\\Files\\Events\\Node\\NodeRenamedEvent', ] as const; -/** Header the subscription carries the shared secret in; the core verifies it. */ -const WEBHOOK_SECRET_HEADER = 'X-TREK-Docsync-Signature'; +/** + * Header the subscription carries the shared secret in. + * + * It has to be the one the endpoint reads — `doc-sync-webhook.controller.ts` + * looks for `x-trek-docsync-secret`. It used to say `X-TREK-Docsync-Signature` + * here, so every webhook Nextcloud actually sent arrived without a secret the + * controller could find and was silently dropped: the subscription existed, the + * calls arrived, and nothing ever came of them. + */ +const WEBHOOK_SECRET_HEADER = 'x-trek-docsync-secret'; /** Where a Nextcloud connection looks for trip folders when nobody said otherwise. */ const DEFAULT_BASE_PATH = '/TREK'; @@ -554,6 +562,11 @@ export class WebdavDocumentProvider implements DocumentProvider { mtimeSeconds: req.mtimeSeconds, sha256: req.sha256, ifMatch: req.expectedRemoteVersion, + // Creating means creating: an unconditional PUT to a name that is + // already taken overwrites somebody else's document without a word. + // Two trips bound to one folder, or a person who put a `receipt.pdf` + // there by hand, are enough for that to happen. + ifNoneMatch: req.remoteId ? undefined : '*', }); // Best effort, and deliberately not fatal: the bytes are already stored, @@ -796,7 +809,12 @@ export class WebdavDocumentProvider implements DocumentProvider { ): Promise { if (remoteId.startsWith(PATH_ID_PREFIX)) { const relative = remoteId.slice(PATH_ID_PREFIX.length); - return relative ? `${resolved.rootPath}/${encodePath(relative)}` : null; + // The path comes out of a listing, which is the provider's word and not + // TREK's. A `..` in it would address a file outside the folder the trip is + // bound to — every other id here is opaque, this one is a path and has to + // be treated like one. + if (!relative || relative.split('/').some(seg => seg === '..' || seg === '.')) return null; + return `${resolved.rootPath}/${encodePath(relative)}`; } const cached = this.hrefCache.get(this.cacheKey(conn, scope))?.get(remoteId); diff --git a/server/tests/unit/nest/doc-sync/doc-sync-webhook.controller.test.ts b/server/tests/unit/nest/doc-sync/doc-sync-webhook.controller.test.ts index a8293212a..1f1581d31 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync-webhook.controller.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync-webhook.controller.test.ts @@ -16,6 +16,8 @@ vi.mock('../../../../src/db/database', () => ({ import { DocSyncWebhookController } from '../../../../src/nest/doc-sync/doc-sync-webhook.controller'; import type { DocSyncConfigService, LinkRow } from '../../../../src/nest/doc-sync/doc-sync-config.service'; import type { DocSyncService } from '../../../../src/nest/doc-sync/doc-sync.service'; +import type { AddonsService } from '../../../../src/nest/addons/addons.service'; +import type { DatabaseService } from '../../../../src/nest/database/database.service'; /** * The webhook endpoint, with both services stubbed. @@ -69,6 +71,15 @@ const config = { webhookSecret: vi.fn(() => SECRET), }; +const settings = new Map(); +const addons = { isAddonEnabled: vi.fn(() => true) }; +const db = { + get: vi.fn((_sql: string, key?: unknown) => { + const value = settings.get(String(key)); + return value === undefined ? undefined : { value }; + }), +}; + const sync = { syncLink: vi.fn(async (_link: LinkRow) => ({ state: 'ok', pulled: 0, pushed: 0, conflicts: 0, missing: 0 })), }; @@ -76,6 +87,8 @@ const sync = { const controller = new DocSyncWebhookController( config as unknown as DocSyncConfigService, sync as unknown as DocSyncService, + addons as unknown as AddonsService, + db as unknown as DatabaseService, ); /** Only what the handler reads: headers, the parsed body, and the raw bytes. */ @@ -105,6 +118,8 @@ function papraReq(payload: string, signature: string, sent = payload): Request { beforeEach(() => { vi.clearAllMocks(); vi.useFakeTimers(); + settings.clear(); + addons.isAddonEnabled.mockReturnValue(true); config.getLinkByToken.mockImplementation((token: string) => (token === 'tok-live' ? link() : undefined)); config.getLink.mockImplementation((id: number) => (id === 4 ? link() : undefined)); config.webhookSecret.mockReturnValue(SECRET); @@ -320,3 +335,39 @@ describe('a burst of nudges', () => { expect(sync.syncLink).not.toHaveBeenCalled(); }); }); + +/** + * The switches an admin expects to mean "off". + * + * The scheduler obeys both the Documents addon and the app_settings kill + * switch; the webhook obeyed neither, so switching document sync off stopped + * the poll while every provider holding a webhook carried on driving full runs. + */ +describe('the admin switches', () => { + it('does nothing while the Documents addon is off', () => { + addons.isAddonEnabled.mockReturnValue(false); + expect(controller.nudge('tok-live', makeReq({ 'x-trek-docsync-secret': SECRET }))).toEqual({ received: true }); + settle(); + expect(sync.syncLink).not.toHaveBeenCalled(); + }); + + it('does nothing while the kill switch is set', () => { + settings.set('docsync_sync_enabled', 'false'); + controller.nudge('tok-live', makeReq({ 'x-trek-docsync-secret': SECRET })); + settle(); + expect(sync.syncLink).not.toHaveBeenCalled(); + }); + + it('drops a scheduled run when the addon goes off inside the window', () => { + controller.nudge('tok-live', makeReq({ 'x-trek-docsync-secret': SECRET })); + addons.isAddonEnabled.mockReturnValue(false); + settle(); + expect(sync.syncLink).not.toHaveBeenCalled(); + }); + + it('runs normally while both are on', () => { + controller.nudge('tok-live', makeReq({ 'x-trek-docsync-secret': SECRET })); + settle(); + expect(sync.syncLink).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/tests/unit/nest/doc-sync/doc-sync.helpers.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.helpers.test.ts index 958b46b50..bbf403eb8 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.helpers.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.helpers.test.ts @@ -489,3 +489,84 @@ describe('planReconcile > upstream reported as unchanged', () => { expect(p.massDeleteGuardTripped).toBe(true); }); }); + +/** + * A download that failed once must be tried again. + * + * `pull()` records the failure together with the remote's current version, so + * the next run computed `remoteChanged = false`; with no local file the other + * branches had nothing to compare and the row fell through to `touch`. Nothing + * in the planner ever planned a second attempt, so the document never arrived, + * the attempt counter never grew past one, and "Sync now" produced the same + * plan. The asymmetry was visible in these tests: a failed PUSH was covered, + * because the TREK-only branch re-plans it; a failed PULL was not. + */ +describe('planReconcile > a pairing whose download never landed', () => { + const failedPull = (over: Partial = {}) => + item({ id: 40, fileId: null, remoteId: 'r1', remoteVersion: 'v1', contentSha256: null, state: 'error', ...over }); + + it('plans another download', () => { + const p = plan({ + items: [failedPull()], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [], + }); + expect(p.actions).toEqual([{ kind: 'pull_update', remote: expect.objectContaining({ remoteId: 'r1' }), itemId: 40 }]); + }); + + it('does not on a push-only binding, which has no business pulling', () => { + const p = plan({ + items: [failedPull()], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [], + direction: 'push', + }); + expect(p.actions.map(a => a.kind)).not.toContain('pull_update'); + }); + + it('leaves a rejected type alone: that answer does not change by trying again', () => { + const p = plan({ + items: [failedPull({ state: 'rejected_type' })], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [], + }); + expect(p.actions.map(a => a.kind)).not.toContain('pull_update'); + }); + + it('leaves an oversized document alone for the same reason', () => { + const p = plan({ + items: [failedPull({ state: 'too_large' })], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [], + }); + expect(p.actions.map(a => a.kind)).not.toContain('pull_update'); + }); + + it('does try a rejected one again once the document upstream changes', () => { + const p = plan({ + items: [failedPull({ state: 'rejected_type' })], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v2' })], + local: [], + }); + expect(p.actions.map(a => a.kind)).toContain('pull_update'); + }); + + it('gives up with the rest once the attempts are spent', () => { + const p = plan({ + items: [failedPull({ attempts: 6 })], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [], + maxAttempts: 6, + }); + expect(p.actions).toEqual([]); + }); + + it('leaves a healthy pairing on touch, not on a pointless re-download', () => { + const p = plan({ + items: [item({ remoteId: 'r1', remoteVersion: 'v1' })], + remote: [remote({ remoteId: 'r1', remoteVersion: 'v1' })], + local: [local({ fileId: 1 })], + }); + expect(p.actions.map(a => a.kind)).toEqual(['touch']); + }); +}); diff --git a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts index 48fe591a4..9d08c82ec 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.service.test.ts @@ -165,6 +165,11 @@ const files = { .run(tripId, file.filename, file.originalname, file.size, file.mimetype, uploadedBy || null); return testDb.prepare('SELECT * FROM trip_files WHERE id = ?').get(info.lastInsertRowid); }), + // Writes for real, like the two above: a double that answers but changes + // nothing lets a missing call pass as a passing test. + softDeleteFile: vi.fn((id: string | number) => { + testDb.prepare('UPDATE trip_files SET deleted_at = CURRENT_TIMESTAMP WHERE id = ?').run(id); + }), } as unknown as FilesService; const realtime = { broadcast: vi.fn() } as unknown as RealtimeService; @@ -927,6 +932,69 @@ describe('DocSyncService', () => { expect(itemRow(itemId).state).toBe('synced'); }); + /** + * The double rename is the only conflict a person can actually produce, and + * it is arbitrated by `remote_name` alone. Clearing a version marker left + * all three names untouched, so the next run raised the same conflict again + * — whatever the owner chose, the issue came back within minutes, forever. + */ + describe('a double rename', () => { + function seedRenameConflict(link: LinkRow) { + const fileId = makeFile({ name: 'Invoice.pdf' }); // renamed in TREK + const itemId = seedItem(link, { + remoteId: 'r1', + remoteName: 'Bill.pdf', // what both agreed on + remoteVersion: 'v1', + fileId, + state: 'conflict', + contentSha256: 'agreed-hash', + }); + return { itemId, fileId }; + } + + it('renames the provider copy to TREK name when TREK wins', async () => { + const link = makeLink(); + const { itemId } = seedRenameConflict(link); + provider.list.mockResolvedValue(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'Invoice.pdf', remoteVersion: 'v2' }), + ]))); + + await service.resolveConflict(itemId, 'trek'); + + expect(provider.rename).toHaveBeenCalledWith( + expect.anything(), expect.anything(), 'r1', 'Invoice.pdf', + ); + expect(itemRow(itemId).remote_name).toBe('Invoice.pdf'); + }); + + it('takes the local rename back when the provider wins, so TREK follows next run', async () => { + const link = makeLink(); + const { itemId, fileId } = seedRenameConflict(link); + provider.list.mockResolvedValue(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'Rechnung.pdf', remoteVersion: 'v1' }), + ]))); + + await service.resolveConflict(itemId, 'provider'); + + const file = testDb.prepare('SELECT original_name FROM trip_files WHERE id = ?').get(fileId) as { original_name: string }; + expect(file.original_name).toBe('Rechnung.pdf'); + }); + + it('does not leave the row in conflict either way', async () => { + for (const keep of ['trek', 'provider'] as const) { + const link = makeLink(); + const { itemId } = seedRenameConflict(link); + provider.list.mockResolvedValue(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'Rechnung.pdf', remoteVersion: 'v1' }), + ]))); + + await service.resolveConflict(itemId, keep); + + expect(itemRow(itemId).state, `keep=${keep} left the row in conflict`).not.toBe('conflict'); + } + }); + }); + it('refuses to resolve a pairing that is not in conflict', async () => { const link = makeLink(); const itemId = seedItem(link, { remoteId: 'r1', state: 'synced' }); @@ -959,6 +1027,165 @@ describe('DocSyncService', () => { * an audit rather than by a failing test, which is why the check lives here, * next to the row it is about. */ + /** + * An upstream edit replaces TREK's copy instead of joining it. + * + * `pull_update` routes into the same `pull()` as a first download, and + * `FilesService.createFile` only inserts — so the edit arrived as a SECOND + * document while the first stayed in the file manager holding the old bytes. + * Worse, the pairing moves to the new row, which leaves the old one with no + * sync item: the next run reads it as a document TREK gained and pushes the + * superseded version back up. One edit, two documents on each side, growing + * with every further edit. + */ + describe('pull_update replaces rather than accumulates', () => { + function seedSynced(link: LinkRow): { itemId: number; fileId: number } { + const fileId = makeFile({ name: 'invoice.pdf' }); + const itemId = seedItem(link, { + remoteId: 'r1', + remoteName: 'invoice.pdf', + remoteVersion: 'v1', + fileId, + state: 'synced', + contentSha256: 'agreed-hash', + }); + return { itemId, fileId }; + } + + it('puts the superseded copy in the trash instead of leaving it beside the new one', async () => { + const link = makeLink(); + const { fileId } = seedSynced(link); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'invoice.pdf', remoteVersion: 'v2' }), + ]))); + + await service.syncLink(link); + + const rows = fileRows(); + const old = rows.find(r => Number(r.id) === fileId) as Record; + const fresh = rows.find(r => Number(r.id) !== fileId) as Record; + expect(old.deleted_at).not.toBeNull(); + expect(fresh).toBeDefined(); + expect(fresh.deleted_at).toBeNull(); + }); + + it('leaves exactly one live document, so the file manager does not show two', async () => { + const link = makeLink(); + seedSynced(link); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'invoice.pdf', remoteVersion: 'v2' }), + ]))); + + await service.syncLink(link); + + expect(fileRows().filter(r => r.deleted_at === null)).toHaveLength(1); + }); + + it('does not push the superseded copy back on the next run', async () => { + const link = makeLink(); + seedSynced(link); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'invoice.pdf', remoteVersion: 'v2' }), + ]))); + await service.syncLink(link); + + provider.push.mockClear(); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r1', name: 'invoice.pdf', remoteVersion: 'v2' }), + ]))); + await service.syncLink(link); + + expect(provider.push).not.toHaveBeenCalled(); + }); + + it('leaves a first download alone: there is nothing to supersede', async () => { + const link = makeLink(); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r-new', name: 'fresh.pdf', remoteVersion: 'v1' }), + ]))); + + await service.syncLink(link); + + expect(fileRows().filter(r => r.deleted_at === null)).toHaveLength(1); + }); + }); + + /** + * The attempt counter and the backoff curve, which were both stuck. + * + * The backoff was always computed at step 1, so three of the curve's four + * steps were unreachable and a provider that was down got asked again at the + * same short interval. And the counter only ever grew: a row that failed once + * a month reached the limit after six months of otherwise healthy syncing and + * was shelved permanently. Driven here through real runs, since the write is + * private and a test hatch would only prove the hatch works. + */ + describe('retry bookkeeping', () => { + /** + * One run in which the download fails, with the backoff window skipped. + * + * The window is real and the planner honours it, so without clearing it the + * second run would correctly skip the row and this would be measuring the + * backoff rather than the counter. + */ + async function failingRun(link: LinkRow) { + testDb.prepare("UPDATE document_sync_items SET next_attempt_at = NULL WHERE remote_id = 'r-flaky'").run(); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r-flaky', name: 'flaky.pdf', remoteVersion: 'v1' }), + ]))); + provider.fetch.mockResolvedValueOnce(fail('provider_error')); + await service.syncLink(link); + } + + /** One run in which it works. */ + async function goodRun(link: LinkRow) { + testDb.prepare("UPDATE document_sync_items SET next_attempt_at = NULL WHERE remote_id = 'r-flaky'").run(); + provider.list.mockResolvedValueOnce(ok(listing([ + remoteDoc({ remoteId: 'r-flaky', name: 'flaky.pdf', remoteVersion: 'v1' }), + ]))); + await service.syncLink(link); + } + + const flakyRow = () => testDb + .prepare("SELECT * FROM document_sync_items WHERE remote_id = 'r-flaky'") + .get() as Record; + + it('counts consecutive failures', async () => { + const link = makeLink(); + await failingRun(link); + expect(flakyRow().attempts).toBe(1); + await failingRun(link); + expect(flakyRow().attempts).toBe(2); + }); + + it('clears the counter once a pass succeeds, so the limit means six in a row', async () => { + const link = makeLink(); + await failingRun(link); + await failingRun(link); + expect(flakyRow().attempts).toBe(2); + + await goodRun(link); + + expect(flakyRow().attempts).toBe(0); + }); + + it('schedules each failure further out than the last', async () => { + const link = makeLink(); + const waits: number[] = []; + for (let i = 0; i < 3; i += 1) { + await failingRun(link); + const row = flakyRow(); + const secs = (testDb + .prepare("SELECT CAST((julianday(?) - julianday('now')) * 86400 AS INTEGER) AS s") + .get(row.next_attempt_at) as { s: number }).s; + waits.push(secs); + } + // Rising rather than flat: the curve used to be indexed at 1 every time. + expect(waits[1]).toBeGreaterThan(waits[0]); + expect(waits[2]).toBeGreaterThan(waits[1]); + }); + }); + describe('resolveConflict across trips', () => { function conflictedItem(linkId: number, tripOfItem: number): number { return Number(testDb.prepare( diff --git a/server/tests/unit/nest/doc-sync/papra.provider.test.ts b/server/tests/unit/nest/doc-sync/papra.provider.test.ts index b98accf5e..90c48249f 100644 --- a/server/tests/unit/nest/doc-sync/papra.provider.test.ts +++ b/server/tests/unit/nest/doc-sync/papra.provider.test.ts @@ -28,6 +28,7 @@ import { tagSearchQuery, } from '../../../../src/nest/doc-sync/providers/papra.client'; import { PapraProvider, buildScopeKey, parseScopeKey } from '../../../../src/nest/doc-sync/providers/papra.provider'; +import { multipartHeader } from '../../../../src/nest/doc-sync/providers/papra.client'; import crypto from 'node:crypto'; import { Readable } from 'node:stream'; @@ -1059,3 +1060,30 @@ describe('the version marker', () => { expect(snapshotVersion(sparse)).toMatch(/^[0-9a-f]{64}$/); }); }); + +/** + * The media type goes into a header, so it has to survive being one. + * + * It comes from `trip_files.mime_type` — influenced by an upload — and went in + * untouched while the filename beside it was escaped. A CR or LF ends the + * header early and lets what follows be read as headers of its own. + */ +describe('multipart part header', () => { + it('keeps an ordinary media type', () => { + expect(multipartHeader('B', 'a.pdf', 'application/pdf').toString()) + .toContain('Content-Type: application/pdf'); + }); + + it('refuses one carrying a newline instead of writing it into the header', () => { + const header = multipartHeader('B', 'a.pdf', 'application/pdf\r\nX-Evil: 1').toString(); + expect(header).not.toContain('X-Evil'); + expect(header).toContain('Content-Type: application/octet-stream'); + }); + + it('falls back for anything that is not a media type at all', () => { + for (const bad of ['', 'nonsense', 'a/b/c', 'a b/c']) { + expect(multipartHeader('B', 'a.pdf', bad).toString()) + .toContain('Content-Type: application/octet-stream'); + } + }); +}); diff --git a/server/tests/unit/nest/doc-sync/webdav.provider.test.ts b/server/tests/unit/nest/doc-sync/webdav.provider.test.ts index 9705be559..d59a33f54 100644 --- a/server/tests/unit/nest/doc-sync/webdav.provider.test.ts +++ b/server/tests/unit/nest/doc-sync/webdav.provider.test.ts @@ -786,7 +786,11 @@ describe('webhooks', () => { expect(bodies[0].get('httpMethod')).toBe('POST'); expect(bodies[0].get('uri')).toBe('https://trek.example/hook'); expect(bodies[0].get('authMethod')).toBe('header'); - expect(bodies[0].get('authData[X-TREK-Docsync-Signature]')).toBe('s3cret'); + // The header the endpoint actually reads. It used to be registered as + // `X-TREK-Docsync-Signature` while doc-sync-webhook.controller looks for + // `x-trek-docsync-secret`, so every webhook Nextcloud sent was dropped + // without a trace: the subscription existed and the calls arrived. + expect(bodies[0].get('authData[x-trek-docsync-secret]')).toBe('s3cret'); // A $regex filter is not merely ignored upstream: it throws inside the // listener and stops delivery for every other webhook on that event. expect(bodies.every((body) => [...body.keys()].every((key) => !key.startsWith('eventFilter')))).toBe(true);