fix(docsync): an idle WebDAV binding read as an emptied folder, and two ids nobody checked

The audit's worst find, reproduced against a live Nextcloud before and after the
fix: the WebDAV adapters answer "nothing changed here" by returning an EMPTY
listing plus `cursorUnchanged`, and nothing read that flag — `grep -rn
cursorUnchanged src/` found it set by four adapters and consulted by none. The
core therefore saw a folder that had lost everything. Run 1 and 2 look fine
because the run's own uploads move the ETag; from run 3 on, every run of an
idle binding ends `partial` with `mass_delete_guard` and six documents reported
missing, forever, until failure_count opens the circuit and the scheduler drops
the binding entirely. That is the ordinary case — a binding nobody is touching.
planReconcile now takes `remoteUnchanged` and plans only what happened in TREK.

Two ids were authorised by trip and never checked against it:

  - resolveConflict took an item id on trust, so the owner of any trip could
    decide a conflict belonging to somebody else's. Both are plain integers and
    nothing compared them.
  - The connection test merged the STORED credential into a probe of whatever
    baseUrl the caller named — a way to have TREK post another account's API
    token at a server of the caller's choosing. Stored secrets are now reused
    only for the address they were stored against.

The phone's connect form was wrong in three ways at once, each of which the
desktop had right: it read the address as `baseUrl` while the field is
`base_url` (so every connection it saved carried an empty address), it drew
`allow_insecure_tls` as a text box and always sent false, and it built label
keys as `docsync.provider${cap(label)}` when the label column already holds the
full suffix — `docsync.providerProviderApiKey`, rendered raw on screen. Those
rules are not markup, so they now live in `useConnectForm`, which both shells
use. Its test fixture invented the field shape and so agreed with the broken
sheet; it now mirrors document_provider_fields.

Also: the scheduler read its interval once at bootstrap while this file's own
comment promised a per-tick read, so changing it did nothing until a restart;
and dueLinks ordered by a column that a successful run clears, which left every
binding tied and SQLite answering in insertion order — on a database with 170
bindings the same twenty ran on every tick and nothing created after them was
ever synced automatically.

server/src/nest/doc-sync/doc-sync.controller.ts

@@ -150,7 +150,13 @@ export class DocSyncController {
     const existing = this.config
       .listConnections(Number(tripId))
       .find((c) => c.provider_id === body.providerId);
-    const stored = existing ? this.config.toRef(existing) : null;
+    // Only for the address the credential was stored against. Merging it into a
+    // probe of an arbitrary baseUrl turns this route into a way to have TREK
+    // post a stored API token at a server of the caller's choosing — which is
+    // exactly what somebody who inherited a trip but not its credentials would
+    // reach for. Same host, same scheme, same port, or the caller types it in.
+    const sameTarget = !!existing && sameOrigin(existing.base_url, urlCheck.data.url);
+    const stored = sameTarget && existing ? this.config.toRef(existing) : null;
     const fields = this.config.providerFields(body.providerId);
     const secrets: Record<string, string> = { ...(stored?.secrets ?? {}) };
     const settings: Record<string, string> = { ...(stored?.settings ?? {}) };
@@ -356,12 +362,25 @@ export class DocSyncController {
     @Body() body: DocsyncResolveConflictDto,
   ) {
     this.assertOwner(tripId, user);
-    const ok = await this.sync.resolveConflict(Number(itemId), body.keep);
+    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 };
   }
 }
 
+
+/** Whether two URLs address the same server, for deciding if a stored secret may be reused. */
+function sameOrigin(a: string | null | undefined, b: string): boolean {
+  if (!a) return false;
+  try {
+    const x = new URL(a);
+    const y = new URL(b);
+    return x.protocol === y.protocol && x.host === y.host;
+  } catch {
+    return false;
+  }
+}
+
 /**
  * The origin a provider has to call back on.
  *

server/src/nest/doc-sync/doc-sync.helpers.ts

@@ -156,6 +156,18 @@ export interface ReconcileInput {
   direction: 'both' | 'pull' | 'push';
   /** A truncated listing must never be read as "the rest was deleted". */
   remoteTruncated: boolean;
+  /**
+   * The provider says its side has not moved since the stored cursor.
+   *
+   * Every adapter derives that cursor from the scope's contents — a digest over
+   * the documents for Paperless, Papra and Synology, the root ETag for WebDAV —
+   * so the flag means "nothing changed upstream" for all of them. The WebDAV
+   * adapter acts on it by returning an EMPTY list instead of walking the folder,
+   * which is only safe if this is read: otherwise a quiet binding looks like a
+   * folder somebody emptied. It was not read, and the third run of every idle
+   * Nextcloud or OpenCloud binding tripped the mass-delete guard for good.
+   */
+  remoteUnchanged: boolean;
   /** Providers without stable ids need the rename heuristic. */
   stableRemoteIds: boolean;
   /** How many failures a row gets before it is shelved for a person to look at. */
@@ -223,6 +235,25 @@ export function planReconcile(input: ReconcileInput): ReconcilePlan {
     if (it.fileId !== null) itemsByFile.set(it.fileId, it);
   }
 
+  // Upstream is unchanged: only what happened in TREK can need doing, and the
+  // remote half of the plan would be reading a list the adapter did not fetch.
+  if (input.remoteUnchanged) {
+    for (const it of items) {
+      if (it.fileId === null || !it.remoteId) continue;
+      const l = localById.get(it.fileId);
+      if (l?.deletedAt) add({ kind: 'local_deleted', itemId: it.id, remoteId: it.remoteId });
+    }
+    if (direction !== 'pull') {
+      for (const l of local) {
+        if (l.deletedAt) continue;
+        const it = itemsByFile.get(l.fileId);
+        if (it?.remoteId) continue;
+        add({ kind: 'push', local: l, itemId: it?.id ?? null, remoteId: null });
+      }
+    }
+    return { actions, massDeleteGuardTripped: false, missingCount: 0 };
+  }
+
   // A listing that lost most of what it had last time is far more likely to be
   // a broken mount, a revoked token or a moved folder than a real mass delete.
   const known = items.filter((i) => i.remoteId && i.state === 'synced');

server/src/nest/doc-sync/doc-sync.job.ts

@@ -30,6 +30,12 @@ import {
  * the Dawarich job on purpose: this one is driven by a per-trip binding an
  * ordinary user creates, and telling them "restart your server" is not an
  * answer.
+ *
+ * Which is why the cron itself runs every minute and the tick decides whether it
+ * is due: baking the interval into the cron expression at bootstrap — as this
+ * did until the claim above was checked against the code — meant a changed
+ * interval did nothing until a restart, quietly, while the admin screen said
+ * otherwise.
  */
 @Injectable()
 export class DocSyncJob implements OnApplicationBootstrap {
@@ -41,12 +47,30 @@ export class DocSyncJob implements OnApplicationBootstrap {
     private readonly registrar: CronRegistrarService,
   ) {}
 
+  /** When the last pass started; null until the first one, which is never skipped. */
+  private lastRunAt: number | null = null;
+
   onApplicationBootstrap(): void {
     if (!this.registrar.isEnabled()) return;
-    const seconds = this.intervalSeconds();
-    const minutes = Math.max(1, Math.round(seconds / 60));
-    logInfo(`Document sync: scheduled every ${minutes}m`);
-    this.registrar.register('docsync', `*/${minutes} * * * *`, () => this.tick());
+    logInfo(`Document sync: polling every ${this.intervalSeconds()}s`);
+    this.registrar.register('docsync', '* * * * *', () => this.tick());
+  }
+
+  /**
+   * Whether enough time has passed for another pass.
+   *
+   * The minute the cron wakes up on is not the unit the setting is in, so a
+   * 90-second interval must not become one minute or two depending on where the
+   * boundaries fall. Comparing against the last run keeps the setting's own
+   * resolution; the cost of the extra wake-ups is one read of app_settings.
+   */
+  private isDue(now: number): boolean {
+    // Null rather than 0: comparing against the epoch means "due" only once the
+    // clock has passed the interval since 1970, which is true in production and
+    // false for any test that picks a small timestamp — a difference that would
+    // have hidden here rather than in the behaviour it is supposed to describe.
+    if (this.lastRunAt === null) return true;
+    return now - this.lastRunAt >= this.intervalSeconds() * 1000;
   }
 
   private intervalSeconds(): number {
@@ -64,6 +88,10 @@ export class DocSyncJob implements OnApplicationBootstrap {
       // default; only an explicit 'false' stops the sync.
       if (killSwitch === 'false') return;
 
+      const now = Date.now();
+      if (!this.isDue(now)) return;
+      this.lastRunAt = now;
+
       // Cheap, and it catches a binding whose owner left the trip through a
       // path that has no hook to attach to — a transfer, a direct DB edit.
       const orphaned = this.config.markOrphanedLinks();

server/src/nest/doc-sync/doc-sync.service.ts

@@ -74,7 +74,17 @@ export class DocSyncService {
 
   // ── Entry points ───────────────────────────────────────────────────────────
 
-  /** Links the scheduler should look at now. */
+  /**
+   * Links the scheduler should look at now, longest-waiting first.
+   *
+   * The second sort key is what makes the limit fair. A successful run clears
+   * `next_attempt_at`, so on a healthy instance every binding sorts equal on the
+   * first key and SQLite falls back to insertion order — which means the twenty
+   * oldest bindings were picked on every tick and everything created after them
+   * was never synced automatically at all. Found on a dev database with 170
+   * bindings, where a freshly created one was still untouched minutes later
+   * while the first twenty ran again and again.
+   */
   dueLinks(limit = 20): LinkRow[] {
     return this.db.connection
       .prepare(
@@ -82,7 +92,9 @@ export class DocSyncService {
           WHERE sync_enabled = 1
             AND failure_count < ?
             AND (next_attempt_at IS NULL OR next_attempt_at <= CURRENT_TIMESTAMP)
-          ORDER BY COALESCE(next_attempt_at, '1970-01-01') ASC
+          ORDER BY COALESCE(next_attempt_at, '1970-01-01') ASC,
+                   COALESCE(last_sync_at, '1970-01-01') ASC,
+                   id ASC
           LIMIT ?`,
       )
       .all(LINK_CIRCUIT_OPEN_AFTER, limit) as LinkRow[];
@@ -161,6 +173,7 @@ export class DocSyncService {
       local,
       direction: link.direction as 'both' | 'pull' | 'push',
       remoteTruncated: listing.data.truncated,
+      remoteUnchanged: listing.data.cursorUnchanged === true,
       stableRemoteIds: provider.capabilities(ref).stableId,
       maxAttempts: ITEM_MAX_ATTEMPTS,
     });
@@ -659,11 +672,21 @@ export class DocSyncService {
    * overwriting either side, which is the only outcome that cannot lose work
    * and is therefore what the UI offers first.
    */
-  async resolveConflict(itemId: number, keep: 'trek' | 'provider' | 'both'): Promise<boolean> {
+  /**
+   * Decide a conflict.
+   *
+   * `tripId` is not decoration: the route authorises the caller against the trip
+   * in its URL, and without checking the row against the same trip an owner of
+   * any trip could resolve a conflict in somebody else's — the id is a plain
+   * 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.
+   */
+  async resolveConflict(itemId: number, keep: 'trek' | 'provider' | 'both', tripId?: number): Promise<boolean> {
     const item = this.db.connection
       .prepare('SELECT * FROM document_sync_items WHERE id = ?')
       .get(itemId) as Record<string, unknown> | undefined;
     if (!item || item.state !== 'conflict') return false;
+    if (tripId !== undefined && Number(item.trip_id) !== Number(tripId)) return false;
     const link = this.config.getLink(Number(item.link_id));
     if (!link) return false;