diff --git a/client/src/api/client.surface.test.ts b/client/src/api/client.surface.test.ts index 474845ec1..eebc8f933 100644 --- a/client/src/api/client.surface.test.ts +++ b/client/src/api/client.surface.test.ts @@ -1,4 +1,4 @@ -// FE-APISURF-001 to FE-APISURF-054 +// FE-APISURF-001 to FE-APISURF-056 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import type { AxiosResponse } from 'axios' import { http, HttpResponse } from 'msw' @@ -10,6 +10,7 @@ import { mapsApi, airportsApi, budgetApi, filesApi, reservationsApi, healthApi, weatherApi, configApi, helpApi, settingsApi, accommodationsApi, dayNotesApi, collabApi, backupApi, shareApi, transitApi, tripInviteApi, notificationsApi, inAppNotificationsApi, memoriesApi, + docsyncApi, } from './client' interface Recorded { method: string; url: string; body: unknown } @@ -665,6 +666,12 @@ describe('client > request payloads', () => { expect(rec.method).toBe('DELETE') expect(rec.body).toEqual({ password: 'hunter2' }) }) + + it('FE-APISURF-056: docsyncApi.createScope names the connection in the path only', async () => { + const rec = await traceOne(() => docsyncApi.createScope(1, 5, 'Norway')) + expect(`${rec.method} ${rec.url}`).toBe('POST /api/trips/1/docsync/connections/5/scopes') + expect(rec.body).toEqual({ name: 'Norway' }) + }) }) describe('client > query parameters', () => { diff --git a/client/src/api/client.ts b/client/src/api/client.ts index c0c03e0e7..4e1f6bf64 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -1375,7 +1375,7 @@ export const docsyncApi = { listScopes: (tripId: number | string, connectionId: number, q?: string) => apiClient.get(`/trips/${tripId}/docsync/connections/${connectionId}/scopes`, { params: q ? { q } : {} }).then(r => r.data), createScope: (tripId: number | string, connectionId: number, name: string) => - apiClient.post(`/trips/${tripId}/docsync/connections/${connectionId}/scopes`, { connectionId, name }).then(r => r.data), + apiClient.post(`/trips/${tripId}/docsync/connections/${connectionId}/scopes`, { name }).then(r => r.data), listLinks: (tripId: number | string) => apiClient.get(`/trips/${tripId}/docsync/links`).then(r => r.data), createLink: (tripId: number | string, data: unknown) => apiClient.post(`/trips/${tripId}/docsync/links`, data).then(r => r.data), updateLink: (tripId: number | string, linkId: number, data: unknown) => apiClient.patch(`/trips/${tripId}/docsync/links/${linkId}`, data).then(r => r.data), diff --git a/server/src/db/document-provider-seed.ts b/server/src/db/document-provider-seed.ts index 0c0ca45b9..d168b1a75 100644 --- a/server/src/db/document-provider-seed.ts +++ b/server/src/db/document-provider-seed.ts @@ -126,26 +126,11 @@ export function seedDocumentProviders(db: Database.Database): void { const insertField = db.prepare( `INSERT OR IGNORE INTO document_provider_fields - (provider_id, field_key, label, input_type, placeholder, hint, required, secret, settings_key, payload_key, sort_order) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + (provider_id, field_key, label, input_type, placeholder, hint, required, secret, sort_order) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, ); for (const f of FIELDS) { - // settings_key and payload_key both carry the field key: every value lands - // in one JSON column here, but the generic client form reads those two and - // would need a branch if they were null. - insertField.run( - f.provider_id, - f.field_key, - f.label, - f.input_type, - f.placeholder, - f.hint, - f.required, - f.secret, - f.secret === 1 ? null : f.field_key, - f.field_key, - f.sort_order, - ); + insertField.run(f.provider_id, f.field_key, f.label, f.input_type, f.placeholder, f.hint, f.required, f.secret, f.sort_order); } } diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index e54fcc8c9..e1dc97a2b 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -4959,10 +4959,12 @@ function runMigrations(db: Database.Database): void { * Journey cascade runs `UPDATE photo_providers SET enabled = 0` with no * WHERE clause, and migrations are append-only — reusing those tables would * change the behaviour of three existing paths, which is exactly what the - * no-breaking-changes rule forbids. The column set is copied verbatim so - * the same generic settings form renders both kinds without a special case, - * including `settings_key`/`payload_key`, which are redundant here (every - * value lands in one JSON column) but keep the client branch-free. + * no-breaking-changes rule forbids. The field columns follow + * `photo_provider_fields` except for `settings_key` and `payload_key`. + * Those map a photo field onto a settings key and a request key; a document + * field is stored under its own `field_key` in one JSON column, and the + * connect form takes its fields from /docsync/providers rather than from + * the generic settings form, so nothing would ever read them. */ () => { db.exec(` @@ -4985,8 +4987,6 @@ function runMigrations(db: Database.Database): void { hint TEXT, required INTEGER DEFAULT 0, secret INTEGER DEFAULT 0, - settings_key TEXT, - payload_key TEXT, sort_order INTEGER DEFAULT 0, UNIQUE(provider_id, field_key) ); diff --git a/server/src/nest/doc-sync/doc-sync-config.service.ts b/server/src/nest/doc-sync/doc-sync-config.service.ts index 0d50cf81c..ed11a531f 100644 --- a/server/src/nest/doc-sync/doc-sync-config.service.ts +++ b/server/src/nest/doc-sync/doc-sync-config.service.ts @@ -1,11 +1,6 @@ import { Injectable } from '@nestjs/common'; import crypto from 'crypto'; -import { - DOCSYNC_SECRET_MASK, - type DocsyncConnectionInput, - type DocsyncLinkInput, - type DocsyncScopeOption, -} from '@trek/shared'; +import { DOCSYNC_SECRET_MASK, type DocsyncConnectionInput, type DocsyncLinkInput } from '@trek/shared'; import { DatabaseService } from '../database/database.service'; import { checkSsrf } from '../../utils/ssrfGuard'; import { DocumentProviderRegistry } from './document-provider.registry'; @@ -315,7 +310,15 @@ export class DocSyncConfigService { }; } - createLink(tripId: number, userId: number, input: DocsyncLinkInput, scope: DocsyncScopeOption | null): DocResult { + /** + * The anchor fields (`remoteRootId`, `remoteRootPath`, `remoteLabel`) are + * what the picker was handed by `listScopes` or `createScope` a moment + * earlier, sent back by the client. Taking them on trust is fine: only the + * trip owner or an instance admin gets here, and an anchor typed in by hand + * still goes through the connection's own credential, so it cannot open + * anything that credential could not open anyway. + */ + createLink(tripId: number, userId: number, input: DocsyncLinkInput): DocResult { const conn = this.getConnection(input.connectionId); if (!conn || conn.trip_id !== tripId) { return { success: false, error: { code: 'not_found', detail: 'connection not found for this trip' } }; @@ -343,9 +346,9 @@ export class DocSyncConfigService { input.connectionId, conn.provider_id, input.scopeKey, - scope?.remoteRootId ?? input.remoteRootId ?? null, - scope?.remoteRootPath ?? input.remoteRootPath ?? null, - scope?.label ?? input.remoteLabel ?? '', + input.remoteRootId ?? null, + input.remoteRootPath ?? null, + input.remoteLabel ?? '', input.direction, input.deletePolicy, input.conflictPolicy, diff --git a/server/src/nest/doc-sync/doc-sync.controller.ts b/server/src/nest/doc-sync/doc-sync.controller.ts index f14e7b750..750bb58f3 100644 --- a/server/src/nest/doc-sync/doc-sync.controller.ts +++ b/server/src/nest/doc-sync/doc-sync.controller.ts @@ -49,8 +49,9 @@ import { * needs a field to render rather than an exception. Everything else answers the * status the failure deserves. * - * Only the trip owner may change a binding. It hands TREK a credential that - * usually reaches the owner's entire document archive, so widening this to + * Only the trip owner may change a binding (plus an instance admin, who + * overrides trip permissions everywhere else too). It hands TREK a credential + * that usually reaches the owner's entire document archive, so widening this to * every member would let any member point the trip at a folder the owner never * meant to share. Reading the status is open to all members — they need to know * where their documents are going. @@ -67,12 +68,16 @@ export class DocSyncController { ) {} /** - * Trip ownership, checked here rather than through @RequirePermission: the + * The trip owner or an instance admin, the same rule as `canManageDocSync` + * on the client. Checked here rather than through @RequirePermission: the * permission table has no "manage integrations" action and inventing one * would grant it to every trip_member by default, which is the opposite of * what a credential this broad needs. + * + * The 403 still says "owner": an admin never sees it, and to everybody who + * does, the owner is the person to ask. */ - private assertOwner(tripId: string, user: User): void { + private assertCanManage(tripId: string, user: User): void { if (user.role === 'admin') return; const trip = this.db.get<{ user_id: number }>('SELECT user_id FROM trips WHERE id = ?', tripId); if (!trip) throw new HttpException('Trip not found', 404); @@ -117,7 +122,7 @@ export class DocSyncController { @CurrentUser() user: User, @Body() body: DocsyncConnectionDto, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); if (!this.config.enabledProviderIds().includes(body.providerId)) { throw new HttpException(`Provider: "${body.providerId}" is not enabled, contact server administrator`, 400); } @@ -138,7 +143,7 @@ export class DocSyncController { @CurrentUser() user: User, @Body() body: DocsyncConnectionTestDto, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const provider = this.registry.get(body.providerId); if (!provider) return { connected: false, error: 'unknown_provider' }; @@ -187,7 +192,7 @@ export class DocSyncController { @Param('connectionId') connectionId: string, @CurrentUser() user: User, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const conn = this.config.getConnection(Number(connectionId)); if (!conn || conn.trip_id !== Number(tripId)) throw new HttpException('Connection not found', 404); this.config.deleteConnection(conn.id); @@ -204,7 +209,7 @@ export class DocSyncController { @CurrentUser() user: User, @Query('q') q?: string, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const conn = this.config.getConnection(Number(connectionId)); if (!conn || conn.trip_id !== Number(tripId)) throw new HttpException('Connection not found', 404); const provider = this.registry.get(conn.provider_id); @@ -221,7 +226,7 @@ export class DocSyncController { @CurrentUser() user: User, @Body() body: DocsyncScopeCreateDto, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const conn = this.config.getConnection(Number(connectionId)); if (!conn || conn.trip_id !== Number(tripId)) throw new HttpException('Connection not found', 404); const provider = this.registry.get(conn.provider_id); @@ -247,8 +252,8 @@ export class DocSyncController { @Body() body: DocsyncLinkDto, @Req() req: Request, ) { - this.assertOwner(tripId, user); - const res = this.config.createLink(Number(tripId), Number(user.id), body, null); + this.assertCanManage(tripId, user); + const res = this.config.createLink(Number(tripId), Number(user.id), body); if (docFailed(res)) throw new HttpException(res.error.detail || res.error.code, 400); // Subscribe where the provider lets TREK do it itself. A failure here is @@ -288,7 +293,7 @@ export class DocSyncController { @Body() body: DocsyncLinkUpdateDto, @Req() req: Request, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const link = this.config.getLink(Number(linkId)); if (!link || link.trip_id !== Number(tripId)) throw new HttpException('Link not found', 404); const updated = this.config.updateLink(link.id, body); @@ -302,7 +307,7 @@ export class DocSyncController { @Param('linkId') linkId: string, @CurrentUser() user: User, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const link = this.config.getLink(Number(linkId)); if (!link || link.trip_id !== Number(tripId)) throw new HttpException('Link not found', 404); @@ -368,7 +373,7 @@ export class DocSyncController { @CurrentUser() user: User, @Body() body: DocsyncResolveConflictDto, ) { - this.assertOwner(tripId, user); + this.assertCanManage(tripId, user); const ok = await this.sync.resolveConflict(Number(itemId), body.keep, Number(tripId)); if (!ok) throw new HttpException('Item is not in conflict', 400); return { success: true }; diff --git a/server/tests/e2e/doc-sync.e2e.test.ts b/server/tests/e2e/doc-sync.e2e.test.ts index 454d8f125..19c8c5ae2 100644 --- a/server/tests/e2e/doc-sync.e2e.test.ts +++ b/server/tests/e2e/doc-sync.e2e.test.ts @@ -267,6 +267,55 @@ describe('Document sync e2e (real guards + real services + temp SQLite)', () => expect(row.secrets).toMatch(/^enc:v1:/); }); + it('creates a folder or tag through the connection the path names', async () => { + const conns = await request(server) + .get(`/api/trips/${tripId}/docsync/connections`) + .set('Cookie', sessionCookie(ownerId)) + .expect(200); + const res = await request(server) + .post(`/api/trips/${tripId}/docsync/connections/${conns.body[0].id}/scopes`) + .set('Cookie', sessionCookie(ownerId)) + .send({ name: 'Norway' }) + .expect(201); + expect(res.body).toMatchObject({ scopeKey: 'tag:2', label: 'Norway' }); + }); + + it('still takes a scope request from a client that sends the connection in the body too', async () => { + // The body field was dropped in favour of the path. A client from before + // that change has it stripped by the contract instead of being refused. + const conns = await request(server) + .get(`/api/trips/${tripId}/docsync/connections`) + .set('Cookie', sessionCookie(ownerId)); + await request(server) + .post(`/api/trips/${tripId}/docsync/connections/${conns.body[0].id}/scopes`) + .set('Cookie', sessionCookie(ownerId)) + .send({ connectionId: conns.body[0].id, name: 'Norway' }) + .expect(201); + }); + + it('creates no scope through a connection that belongs to another trip', async () => { + const otherTripId = createTrip(db as never, ownerId, { title: 'Elsewhere' }).id; + const conns = await request(server) + .get(`/api/trips/${tripId}/docsync/connections`) + .set('Cookie', sessionCookie(ownerId)); + await request(server) + .post(`/api/trips/${otherTripId}/docsync/connections/${conns.body[0].id}/scopes`) + .set('Cookie', sessionCookie(ownerId)) + .send({ name: 'Norway' }) + .expect(404); + }); + + it('does not let a plain member create a folder or tag', async () => { + const conns = await request(server) + .get(`/api/trips/${tripId}/docsync/connections`) + .set('Cookie', sessionCookie(ownerId)); + await request(server) + .post(`/api/trips/${tripId}/docsync/connections/${conns.body[0].id}/scopes`) + .set('Cookie', sessionCookie(memberId)) + .send({ name: 'Norway' }) + .expect(403); + }); + it('binds the trip to a scope and reports it to every member', async () => { const conns = await request(server) .get(`/api/trips/${tripId}/docsync/connections`) @@ -291,6 +340,34 @@ describe('Document sync e2e (real guards + real services + temp SQLite)', () => expect(asMember.body[0].webhookSecret).toBe('••••••••'); }); + it('changes only the setting a patch names, through the real contract', async () => { + // The unit tests call the handler directly and never see the pipe. Through + // it, a schema that fills in defaults turns one switch into a full reset. + const links = await request(server) + .get(`/api/trips/${tripId}/docsync/links`) + .set('Cookie', sessionCookie(ownerId)); + const linkId = links.body[0].id; + await request(server) + .patch(`/api/trips/${tripId}/docsync/links/${linkId}`) + .set('Cookie', sessionCookie(ownerId)) + .send({ direction: 'pull', deletePolicy: 'trash' }) + .expect(200); + + const paused = await request(server) + .patch(`/api/trips/${tripId}/docsync/links/${linkId}`) + .set('Cookie', sessionCookie(ownerId)) + .send({ syncEnabled: false }) + .expect(200); + + expect(paused.body).toMatchObject({ + syncEnabled: false, + direction: 'pull', + deletePolicy: 'trash', + conflictPolicy: 'manual', + remoteLabel: 'Japan 2026', + }); + }); + it('refuses a binding whose connection belongs to another trip', async () => { const otherTripId = createTrip(db as never, ownerId, { title: 'Other' }).id; const conns = await request(server) diff --git a/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts b/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts index 52e76c638..d009fd6b7 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync-config.service.test.ts @@ -106,7 +106,7 @@ function linkInput(connectionId: number, over: Partial = {}): } function link(connectionId: number, over: Partial = {}): LinkRow { - const result = svc.createLink(TRIP, OWNER, linkInput(connectionId, over), null); + const result = svc.createLink(TRIP, OWNER, linkInput(connectionId, over)); if (docFailed(result)) throw new Error(`fixture link refused: ${JSON.stringify(result.error)}`); return result.data; } @@ -363,12 +363,11 @@ describe('toScopeRef', () => { // The adapters take a ref object rather than positional arguments because // an id and a path swapped compile cleanly and then read the wrong folder. const conn = await connect(); - const created = svc.createLink(TRIP, OWNER, linkInput(conn.id, { scopeKey: 'fileid:437' }), { - scopeKey: 'fileid:437', - label: 'Japan 2026', - remoteRootId: '437', - remoteRootPath: '/TREK/japan', - }); + const created = svc.createLink( + TRIP, + OWNER, + linkInput(conn.id, { scopeKey: 'fileid:437', remoteRootId: '437', remoteRootPath: '/TREK/japan' }), + ); expect(created.success).toBe(true); const row = created.success ? created.data : ({} as LinkRow); testDb.prepare('UPDATE trip_document_links SET remote_cursor = ? WHERE id = ?').run('etag-9', row.id); @@ -388,7 +387,7 @@ describe('createLink', () => { it('returns the binding that already exists rather than making a second one', async () => { const conn = await connect(); const first = link(conn.id); - const again = svc.createLink(TRIP, MEMBER, linkInput(conn.id, { remoteLabel: 'renamed' }), null); + const again = svc.createLink(TRIP, MEMBER, linkInput(conn.id, { remoteLabel: 'renamed' })); expect(again.success && again.data.id).toBe(first.id); expect(svc.listLinks(TRIP)).toHaveLength(1); }); @@ -404,32 +403,39 @@ describe('createLink', () => { it('refuses a connection that belongs to somebody else\'s trip', async () => { const otherTrip = createTrip(testDb, MEMBER, { title: 'Not yours' }).id; const conn = await connect(); - const result = svc.createLink(otherTrip, MEMBER, linkInput(conn.id), null); + const result = svc.createLink(otherTrip, MEMBER, linkInput(conn.id)); expect(result.success).toBe(false); expect(result.success === false && result.error.code).toBe('not_found'); expect(svc.listLinks(otherTrip)).toHaveLength(0); }); it('refuses a connection id that does not exist at all', async () => { - const result = svc.createLink(TRIP, OWNER, linkInput(9999), null); + const result = svc.createLink(TRIP, OWNER, linkInput(9999)); expect(result.success).toBe(false); }); - it('takes the anchor from the scope the user picked, not from what the client sent', async () => { + it('stores the anchor the picker sent back with the scope key', async () => { const conn = await connect(); const result = svc.createLink( TRIP, OWNER, - linkInput(conn.id, { remoteRootId: 'stale', remoteRootPath: '/stale', remoteLabel: 'stale' }), - { scopeKey: 'tag:1', label: 'Japan 2026', remoteRootId: '17', remoteRootPath: '/TREK/japan' }, + linkInput(conn.id, { remoteRootId: '17', remoteRootPath: '/TREK/japan', remoteLabel: 'Japan 2026' }), ); expect(result.success && result.data).toMatchObject({ + remote_scope_key: 'tag:1', remote_root_id: '17', remote_root_path: '/TREK/japan', remote_label: 'Japan 2026', }); }); + it('stores NULL for an anchor field the client left out', async () => { + // Synology binds by path alone, so its options carry no root id at all. + const conn = await connect(); + const result = svc.createLink(TRIP, OWNER, linkInput(conn.id)); + expect(result.success && result.data).toMatchObject({ remote_root_id: null, remote_root_path: null }); + }); + it('gives every binding its own webhook token and keeps the secret out of the public view', async () => { // A leaked webhook URL has to identify exactly one binding, so revoking one // cannot be used to poke another. diff --git a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts index 5bb5b8028..141600d76 100644 --- a/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts +++ b/server/tests/unit/nest/doc-sync/doc-sync.controller.test.ts @@ -440,14 +440,24 @@ describe('the scope picker', () => { data: { scopeKey: 'tag:9', label: 'Norway', remoteRootId: '9', remoteRootPath: '/TREK/norway' }, } as never); - await expect(controller.createScope(String(tripId), String(conn.id), owner, { connectionId: conn.id, name: 'Norway' })) + await expect(controller.createScope(String(tripId), String(conn.id), owner, { name: 'Norway' })) .resolves.toMatchObject({ scopeKey: 'tag:9', label: 'Norway' }); + expect(paperless.createScope).toHaveBeenCalledWith(expect.objectContaining({ connectionId: conn.id }), 'Norway'); + }); + + it('creates nothing through a connection that belongs to a different trip', async () => { + // The connection comes from the path and nowhere else, so the path is what + // has to match the trip the caller was admitted to. + const conn = await storedPaperless(); + const err = await thrown(() => controller.createScope(String(otherTripId), String(conn.id), admin, { name: 'Norway' })); + expect(err.getStatus()).toBe(404); + expect(paperless.createScope).not.toHaveBeenCalled(); }); it('turns a refused creation into a 400 carrying the provider code', async () => { const conn = await storedPaperless(); paperless.createScope.mockResolvedValueOnce({ success: false, error: { code: 'forbidden' } } as never); - const err = await thrown(() => controller.createScope(String(tripId), String(conn.id), owner, { connectionId: conn.id, name: 'Norway' })); + const err = await thrown(() => controller.createScope(String(tripId), String(conn.id), owner, { name: 'Norway' })); expect(err.getStatus()).toBe(400); expect(err.message).toBe('forbidden'); }); @@ -695,9 +705,9 @@ describe('a manual run and the shelved rows', () => { /** * The trip the route was authorised against is now passed on to the service. * - * `assertOwner` proves the caller owns the trip in the URL and says nothing - * about whether the item id in the path belongs to that trip — the id is a - * plain integer, and nothing tied the two together. The check itself lives in + * `assertCanManage` proves the caller may manage the trip in the URL and says + * nothing about whether the item id in the path belongs to that trip: the id is + * a plain integer, and nothing tied the two together. The check itself lives in * the service (where the row is), so what belongs here is that the trip reaches * it at all. */ diff --git a/shared/src/docsync/docsync.schema.spec.ts b/shared/src/docsync/docsync.schema.spec.ts new file mode 100644 index 000000000..d97052538 --- /dev/null +++ b/shared/src/docsync/docsync.schema.spec.ts @@ -0,0 +1,62 @@ +import { docsyncLinkInputSchema, docsyncLinkUpdateSchema, docsyncScopeCreateSchema } from './docsync.schema'; + +import { describe, it, expect } from 'vitest'; + +describe('docsyncScopeCreateSchema', () => { + it('takes only a name, because the connection is in the path', () => { + expect(docsyncScopeCreateSchema.parse({ name: ' Norway ' })).toEqual({ name: 'Norway' }); + }); + + it('strips a connection id sent in the body instead of refusing the request', () => { + expect(docsyncScopeCreateSchema.parse({ connectionId: 5, name: 'Norway' })).toEqual({ name: 'Norway' }); + }); + + it('rejects a blank or oversized name', () => { + expect(docsyncScopeCreateSchema.safeParse({ name: ' ' }).success).toBe(false); + expect(docsyncScopeCreateSchema.safeParse({ name: 'x'.repeat(201) }).success).toBe(false); + expect(docsyncScopeCreateSchema.safeParse({}).success).toBe(false); + }); +}); + +describe('docsyncLinkInputSchema', () => { + it('needs the connection in the body, since POST /links has no connection in its path', () => { + expect(docsyncLinkInputSchema.safeParse({ scopeKey: 'tag:1' }).success).toBe(false); + }); + + it('fills the safe defaults for everything the picker leaves out', () => { + expect(docsyncLinkInputSchema.parse({ connectionId: 3, scopeKey: 'tag:1' })).toEqual({ + connectionId: 3, + scopeKey: 'tag:1', + remoteLabel: '', + direction: 'both', + deletePolicy: 'unlink', + conflictPolicy: 'manual', + syncEnabled: true, + }); + }); +}); + +describe('docsyncLinkUpdateSchema', () => { + it('passes on only the fields the patch named, with no defaults filled in', () => { + // Defaults here would reach updateLink as real values: pausing a pull-only + // binding used to switch it to two-way and blank its label. + expect(docsyncLinkUpdateSchema.parse({ syncEnabled: false })).toEqual({ syncEnabled: false }); + expect(docsyncLinkUpdateSchema.parse({})).toEqual({}); + }); + + it('still checks the values it is given', () => { + expect(docsyncLinkUpdateSchema.safeParse({ direction: 'sideways' }).success).toBe(false); + expect(docsyncLinkUpdateSchema.safeParse({ syncEnabled: 'no' }).success).toBe(false); + }); + + it('drops the connection and the anchor, which are fixed once a binding exists', () => { + const parsed = docsyncLinkUpdateSchema.parse({ + connectionId: 9, + scopeKey: 'tag:2', + remoteRootId: '2', + remoteRootPath: '/elsewhere', + deletePolicy: 'trash', + }); + expect(parsed).toEqual({ deletePolicy: 'trash' }); + }); +}); diff --git a/shared/src/docsync/docsync.schema.ts b/shared/src/docsync/docsync.schema.ts index b35dc997d..0ccf74eeb 100644 --- a/shared/src/docsync/docsync.schema.ts +++ b/shared/src/docsync/docsync.schema.ts @@ -105,9 +105,13 @@ export type DocsyncScopeOption = z.infer; * Creating a scope rather than picking one. Only providers with a container * concept accept this (a WebDAV folder, an OpenCloud space, a Paperless tag, * a Papra tag); the adapter reports whether it can. + * + * The connection comes from the path (`connections/:connectionId/scopes`) and + * is not repeated here. A copy in the body was never read and only let one + * request name two different connections; a client that still sends it has + * the key stripped rather than refused. */ export const docsyncScopeCreateSchema = z.object({ - connectionId: idSchema, /** Name for the new folder, tag or space. */ name: z.string().trim().min(1).max(200), }); @@ -155,9 +159,22 @@ export type DocsyncLinkInput = z.infer; * it would leave them all pointing somewhere else. The root fields are left out * rather than ignored — they were accepted and silently dropped, so a client * sending one was told the move had happened. + * + * Spelled out instead of derived from the input schema with `.partial()`. In + * Zod 4 a `.default()` still fires inside an optional field, so the derived + * version filled in every default the patch left out: `{ syncEnabled: false }` + * arrived as a full set of settings and reset direction, both policies and the + * label with it. A pull-only binding turned two-way the moment somebody paused + * it. */ -export const docsyncLinkUpdateSchema = docsyncLinkInputSchema - .omit({ connectionId: true, scopeKey: true, remoteRootId: true, remoteRootPath: true }) +export const docsyncLinkUpdateSchema = z + .object({ + remoteLabel: z.string().max(512), + direction: docsyncDirectionSchema, + deletePolicy: docsyncDeletePolicySchema, + conflictPolicy: docsyncConflictPolicySchema, + syncEnabled: z.boolean(), + }) .partial(); // ── Sync state ───────────────────────────────────────────────────────────────