fix(offline,server,docs): apply PR #116 review findings to offline-sync

Carries the still-applicable findings from the PR #116 review into PR #120,
since #120 includes the mobile-bootstrap commit. CORS hardening (removing the
unconditional localhost/capacitor origins) is intentionally left out of scope.

Service worker routing (latent bug fix + testability):
- vite.config.ts: anchor Workbox path matching to a segment boundary
  (^/<seg>(/|$)) instead of startsWith, so siblings like /apidocs,
  /collaborators, /socket.iox are no longer mis-routed as API/realtime and
  forced NetworkOnly; align navigateFallbackDenylist with the same anchors.
- new apps/client/src/pwa/sw-strategy.ts holds the canonical predicates
  (isApiPath, isCollabOrSocketPath) + unit tests; the vite.config regexes
  mirror it inline (Workbox generateSW serializes urlPattern fns standalone,
  so they cannot import the module).

Server CORS (R1 extraction + coverage):
- extract buildCorsAllowlist / isOriginAllowed into cors.util.ts with unit
  tests (evil-origin rejected, WebView/no-Origin allowed); main.ts rewired to
  use them with byte-for-byte identical behavior.

Privacy — clear offline cache on logout:
- new clear-offline-cache.ts purges the persisted query cache
  (idb-keyval gitmost-rq-cache), the Yjs page.* IndexedDB databases, and the
  service-worker api-get-cache; wired into handleLogout (best-effort, before
  the redirect) so a previous user's private data does not linger locally.

Conventions & docs:
- prettier fixes on main.ts and login.dto.ts.
- CHANGELOG: document offline reading, returnToken opt-in, optional Swagger,
  new env vars, logout cache-clear, and the CORS open->allowlist breaking
  change.
- docs/mobile-app-plan.md: correct the now-false §2.4 claims and update the
  §12 checklist (native cap add ios left unchecked — generated locally,
  gitignored).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

apps/client/src/features/auth/hooks/use-auth.ts

@@ -23,6 +23,7 @@ import { acceptInvitation } from "@/features/workspace/services/workspace-servic
 import APP_ROUTE, { getPostLoginRedirect } from "@/lib/app-route.ts";
 import { RESET } from "jotai/utils";
 import { useTranslation } from "react-i18next";
+import { clearOfflineCache } from "@/features/offline/clear-offline-cache";
 
 export default function useAuth() {
   const { t } = useTranslation();
@@ -123,6 +124,13 @@ export default function useAuth() {
   const handleLogout = async () => {
     setCurrentUser(RESET);
     await logout();
+    // Purge the previous user's offline data while the page is still alive —
+    // window.location.replace below would otherwise interrupt async cleanup.
+    try {
+      await clearOfflineCache();
+    } catch {
+      // best-effort: never block logout on cache cleanup
+    }
     window.location.replace(`${APP_ROUTE.AUTH.LOGIN}?logout=1`);
   };
 

apps/client/src/features/offline/clear-offline-cache.test.ts

@@ -0,0 +1,107 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+
+// vi.mock factories are hoisted above imports, so the spies they reference must
+// be declared via vi.hoisted (also hoisted). These are inspected by assertions.
+const h = vi.hoisted(() => ({
+  clear: vi.fn(),
+  del: vi.fn(),
+}));
+
+// The module under test imports the app entry at load time — it must be mocked.
+vi.mock("@/main.tsx", () => ({
+  queryClient: { clear: h.clear },
+}));
+vi.mock("idb-keyval", () => ({
+  del: h.del,
+}));
+
+import { clearOfflineCache } from "./clear-offline-cache";
+import { OFFLINE_CACHE_KEY } from "./query-persister";
+
+// jsdom does not provide indexedDB.databases() or Cache Storage, so the browser
+// globals are stubbed per-test. We restore them afterwards.
+const originalIndexedDB = (globalThis as any).indexedDB;
+const originalCaches = (globalThis as any).caches;
+
+beforeEach(() => {
+  h.clear.mockClear();
+  h.del.mockClear();
+});
+
+afterEach(() => {
+  (globalThis as any).indexedDB = originalIndexedDB;
+  (globalThis as any).caches = originalCaches;
+  vi.restoreAllMocks();
+});
+
+describe("clearOfflineCache", () => {
+  it("resolves without throwing when the browser globals are absent", async () => {
+    (globalThis as any).indexedDB = undefined;
+    delete (globalThis as any).caches;
+
+    await expect(clearOfflineCache()).resolves.toBeUndefined();
+
+    // The two store-agnostic steps still run.
+    expect(h.clear).toHaveBeenCalledTimes(1);
+    expect(h.del).toHaveBeenCalledWith(OFFLINE_CACHE_KEY);
+  });
+
+  it("deletes only `page.*` IndexedDB databases and only `api-get-cache` caches", async () => {
+    const deleteDatabase = vi.fn((_name: string) => {
+      const request: any = {};
+      // Resolve the deletion on the next microtask, like a real IDBRequest.
+      queueMicrotask(() => request.onsuccess && request.onsuccess());
+      return request;
+    });
+    (globalThis as any).indexedDB = {
+      databases: vi
+        .fn()
+        .mockResolvedValue([
+          { name: "page.aaa" },
+          { name: "page.bbb" },
+          { name: "keyval-store" },
+          { name: undefined },
+        ]),
+      deleteDatabase,
+    };
+
+    const cacheDelete = vi.fn().mockResolvedValue(true);
+    (globalThis as any).caches = {
+      keys: vi
+        .fn()
+        .mockResolvedValue([
+          "workbox-runtime-https://app/api-get-cache",
+          "other-cache",
+        ]),
+      delete: cacheDelete,
+    };
+
+    await expect(clearOfflineCache()).resolves.toBeUndefined();
+
+    // Only the two page.* databases are deleted.
+    expect(deleteDatabase).toHaveBeenCalledTimes(2);
+    expect(deleteDatabase).toHaveBeenCalledWith("page.aaa");
+    expect(deleteDatabase).toHaveBeenCalledWith("page.bbb");
+
+    // Only the api-get-cache entry is deleted.
+    expect(cacheDelete).toHaveBeenCalledTimes(1);
+    expect(cacheDelete).toHaveBeenCalledWith(
+      "workbox-runtime-https://app/api-get-cache",
+    );
+  });
+
+  it("never throws even if a step rejects (best-effort)", async () => {
+    h.del.mockRejectedValueOnce(new Error("idb boom"));
+    (globalThis as any).indexedDB = {
+      databases: vi.fn().mockRejectedValue(new Error("databases boom")),
+      deleteDatabase: vi.fn(),
+    };
+    (globalThis as any).caches = {
+      keys: vi.fn().mockRejectedValue(new Error("caches boom")),
+      delete: vi.fn(),
+    };
+
+    await expect(clearOfflineCache()).resolves.toBeUndefined();
+    expect(h.clear).toHaveBeenCalledTimes(1);
+  });
+});

apps/client/src/features/offline/clear-offline-cache.ts

@@ -0,0 +1,90 @@
+import { del } from "idb-keyval";
+
+import { queryClient } from "@/main.tsx";
+import { OFFLINE_CACHE_KEY } from "./query-persister";
+
+/**
+ * Best-effort purge of all of the current user's offline data from the browser.
+ *
+ * On logout the previous user's private data would otherwise linger locally and
+ * be readable by the next person on the device. This clears the three offline
+ * stores the app writes:
+ *   1. the in-memory + IndexedDB-persisted TanStack Query cache (idb-keyval key
+ *      `OFFLINE_CACHE_KEY`),
+ *   2. the Yjs page documents (IndexedDB databases named `page.<id>` created by
+ *      y-indexeddb in make-offline.ts), and
+ *   3. the service worker `api-get-cache` Cache Storage entry (private GET /api
+ *      responses cached by the Workbox runtime).
+ *
+ * Fully best-effort: every step is isolated so a single failure neither blocks
+ * the remaining steps nor throws to the caller (logout must never be blocked on
+ * cache cleanup). Callers may ignore the resolved value.
+ *
+ * Limitations:
+ *   - Deleting the Yjs page databases relies on `indexedDB.databases()`, which
+ *     is unavailable in some browsers (notably Firefox). There we skip silently;
+ *     those `page.<id>` databases are then left in place.
+ *   - Cache Storage clearing only runs where `caches` exists (secure contexts /
+ *     service-worker-capable browsers).
+ */
+export async function clearOfflineCache(): Promise<void> {
+  // 1a. Drop the in-memory query cache immediately.
+  try {
+    queryClient.clear();
+  } catch {
+    // best-effort: ignore in-memory cache reset failures
+  }
+
+  // 1b. Delete the persisted RQ cache from IndexedDB.
+  try {
+    await del(OFFLINE_CACHE_KEY);
+  } catch {
+    // best-effort: ignore persisted-cache deletion failures
+  }
+
+  // 2. Delete the Yjs page IndexedDB databases (`page.<id>`).
+  // `indexedDB.databases()` is not implemented everywhere (e.g. Firefox); when
+  // it is missing we cannot enumerate the page databases, so we skip silently.
+  try {
+    if (
+      typeof indexedDB !== "undefined" &&
+      typeof indexedDB.databases === "function"
+    ) {
+      const dbs = await indexedDB.databases();
+      for (const db of dbs) {
+        const name = db?.name;
+        if (typeof name !== "string" || !name.startsWith("page.")) continue;
+        try {
+          // Fire-and-forget delete; await a thin wrapper so a slow delete does
+          // not race the page teardown, but never reject on it.
+          await new Promise<void>((resolve) => {
+            const request = indexedDB.deleteDatabase(name);
+            request.onsuccess = () => resolve();
+            request.onerror = () => resolve();
+            request.onblocked = () => resolve();
+          });
+        } catch {
+          // best-effort per database
+        }
+      }
+    }
+  } catch {
+    // best-effort: ignore enumeration/deletion failures
+  }
+
+  // 3. Clear the service worker API cache (private GET /api responses). The
+  // Workbox runtime cache name contains "api-get-cache" (Workbox may prefix it),
+  // so match by substring rather than exact name.
+  try {
+    if ("caches" in window) {
+      const keys = await caches.keys();
+      await Promise.all(
+        keys
+          .filter((key) => key.includes("api-get-cache"))
+          .map((key) => caches.delete(key)),
+      );
+    }
+  } catch {
+    // best-effort: ignore Cache Storage failures
+  }
+}

apps/client/src/features/offline/query-persister.ts

@@ -11,6 +11,11 @@ type DehydratableQuery = {
   queryKey: readonly unknown[];
 };
 
+// idb-keyval key under which TanStack Query persists its dehydrated cache.
+// Exported so the logout cache-clear logic deletes the exact same key (no
+// magic-string drift between persist and purge).
+export const OFFLINE_CACHE_KEY = "gitmost-rq-cache";
+
 // IndexedDB-backed storage adapter for TanStack Query's async persister.
 const idbStorage = {
   getItem: (key: string) => get<string>(key).then((v) => v ?? null),
@@ -20,7 +25,7 @@ const idbStorage = {
 
 export const queryPersister = createAsyncStoragePersister({
   storage: idbStorage,
-  key: "gitmost-rq-cache",
+  key: OFFLINE_CACHE_KEY,
   throttleTime: 1000,
 });
 

apps/client/src/pwa/sw-strategy.test.ts

@@ -0,0 +1,32 @@
+import { describe, it, expect } from "vitest";
+import { isApiPath, isCollabOrSocketPath } from "./sw-strategy";
+
+describe("isApiPath", () => {
+  it("matches the /api segment and its subtree", () => {
+    expect(isApiPath("/api")).toBe(true);
+    expect(isApiPath("/api/")).toBe(true);
+    expect(isApiPath("/api/pages")).toBe(true);
+  });
+
+  it("does not over-match sibling paths", () => {
+    expect(isApiPath("/apidocs")).toBe(false);
+    expect(isApiPath("/apixyz")).toBe(false);
+    expect(isApiPath("/")).toBe(false);
+    expect(isApiPath("/pages")).toBe(false);
+  });
+});
+
+describe("isCollabOrSocketPath", () => {
+  it("matches the /collab and /socket.io segments and their subtrees", () => {
+    expect(isCollabOrSocketPath("/collab")).toBe(true);
+    expect(isCollabOrSocketPath("/collab/x")).toBe(true);
+    expect(isCollabOrSocketPath("/socket.io")).toBe(true);
+    expect(isCollabOrSocketPath("/socket.io/abc")).toBe(true);
+  });
+
+  it("does not over-match sibling paths", () => {
+    expect(isCollabOrSocketPath("/collaborators")).toBe(false);
+    expect(isCollabOrSocketPath("/collabx")).toBe(false);
+    expect(isCollabOrSocketPath("/socket.iox")).toBe(false);
+  });
+});

apps/client/src/pwa/sw-strategy.ts

@@ -0,0 +1,32 @@
+/**
+ * Canonical service-worker routing predicates.
+ *
+ * IMPORTANT: With vite-plugin-pwa using Workbox `generateSW`, the
+ * `runtimeCaching[].urlPattern` functions are serialized standalone into the
+ * generated service worker and CANNOT reference imported symbols. The matching
+ * logic is therefore duplicated as inline regex literals in
+ * apps/client/vite.config.ts. This module is the testable source of truth, and
+ * the two MUST be kept in sync. This duplication is intentional and is the
+ * documented Workbox limitation.
+ *
+ * Matching is anchored to a path SEGMENT boundary (`^/<seg>(/|$)`) so that
+ * sibling paths like `/apidocs`, `/collaborators`, `/socket.iox` are NOT
+ * wrongly treated as API/realtime traffic.
+ */
+
+/**
+ * True when `pathname` is the `/api` segment or anything beneath it.
+ * `/api` and `/api/...` -> true; `/apidocs`, `/apixyz` -> false.
+ */
+export function isApiPath(pathname: string): boolean {
+  return /^\/api(\/|$)/.test(pathname);
+}
+
+/**
+ * True when `pathname` is the `/collab` or `/socket.io` segment (or beneath it).
+ * `/collab`, `/collab/x`, `/socket.io`, `/socket.io/abc` -> true;
+ * `/collaborators`, `/collabx`, `/socket.iox` -> false.
+ */
+export function isCollabOrSocketPath(pathname: string): boolean {
+  return /^\/(collab|socket\.io)(\/|$)/.test(pathname);
+}

apps/client/vite.config.ts

@@ -64,17 +64,24 @@ export default defineConfig(({ mode }) => {
         workbox: {
           globPatterns: ["**/*.{js,css,html,svg,png,ico,woff2,json}"],
           navigateFallback: "index.html",
-          navigateFallbackDenylist: [/^\/api\//, /^\/collab\//, /^\/socket\.io\//],
+          // Segment-anchored (`^/<seg>(/|$)`) so navigation requests to these
+          // segments are consistently excluded from the SPA fallback, mirroring
+          // the runtimeCaching urlPattern regexes below.
+          navigateFallbackDenylist: [/^\/api(\/|$)/, /^\/collab(\/|$)/, /^\/socket\.io(\/|$)/],
           cleanupOutdatedCaches: true,
           clientsClaim: true,
+          // The urlPattern regexes below mirror apps/client/src/pwa/sw-strategy.ts
+          // and MUST be kept in sync with it. Workbox `generateSW` serializes these
+          // functions standalone into the generated service worker, so they cannot
+          // import the module — the matching logic is intentionally duplicated as
+          // self-contained inline regex literals anchored to a path segment boundary.
           runtimeCaching: [
-            { urlPattern: ({ url }) => url.pathname.startsWith("/collab"), handler: "NetworkOnly" },
-            { urlPattern: ({ url }) => url.pathname.startsWith("/socket.io"), handler: "NetworkOnly" },
+            { urlPattern: ({ url }) => /^\/(collab|socket\.io)(\/|$)/.test(url.pathname), handler: "NetworkOnly" },
             // M2 read-path: GET navigation API responses fall back to cache when offline.
             // Only GET is cached; mutations always hit the network (Workbox caching handlers
             // only match GET by default, but scope explicitly for clarity/safety).
             {
-              urlPattern: ({ url, request }) => url.pathname.startsWith("/api") && request.method === "GET",
+              urlPattern: ({ url, request }) => /^\/api(\/|$)/.test(url.pathname) && request.method === "GET",
               handler: "NetworkFirst",
               options: {
                 cacheName: "api-get-cache",
@@ -83,7 +90,7 @@ export default defineConfig(({ mode }) => {
               },
             },
             // Any non-GET /api stays network-only (never served stale).
-            { urlPattern: ({ url }) => url.pathname.startsWith("/api"), handler: "NetworkOnly" },
+            { urlPattern: ({ url }) => /^\/api(\/|$)/.test(url.pathname), handler: "NetworkOnly" },
           ],
         },
         devOptions: { enabled: false },

apps/server/src/core/auth/dto/login.dto.ts

@@ -1,4 +1,10 @@
-import { IsBoolean, IsEmail, IsNotEmpty, IsOptional, IsString } from 'class-validator';
+import {
+  IsBoolean,
+  IsEmail,
+  IsNotEmpty,
+  IsOptional,
+  IsString,
+} from 'class-validator';
 
 export class LoginDto {
   @IsNotEmpty()

apps/server/src/integrations/environment/cors.util.spec.ts

@@ -0,0 +1,87 @@
+import { buildCorsAllowlist, isOriginAllowed } from './cors.util';
+
+const WEBVIEW_ORIGINS = [
+  'capacitor://localhost',
+  'ionic://localhost',
+  'http://localhost',
+  'https://localhost',
+];
+
+describe('isOriginAllowed', () => {
+  const allowlist = buildCorsAllowlist({
+    appUrl: 'https://app.example',
+    configuredOrigins: ['https://other.example'],
+  });
+
+  it('allows requests with no Origin header', () => {
+    expect(isOriginAllowed(undefined, allowlist)).toBe(true);
+    expect(isOriginAllowed('', allowlist)).toBe(true);
+  });
+
+  it('allows an exact allowlisted origin', () => {
+    expect(isOriginAllowed('https://app.example', allowlist)).toBe(true);
+    expect(isOriginAllowed('https://other.example', allowlist)).toBe(true);
+  });
+
+  it('allows each native WebView origin', () => {
+    for (const origin of WEBVIEW_ORIGINS) {
+      expect(isOriginAllowed(origin, allowlist)).toBe(true);
+    }
+  });
+
+  it('rejects a foreign credentialed origin', () => {
+    // With credentials:true a foreign credentialed origin must be rejected.
+    expect(isOriginAllowed('https://evil.example', allowlist)).toBe(false);
+  });
+
+  it('rejects a trailing-slash mismatch', () => {
+    expect(isOriginAllowed('https://app.example/', allowlist)).toBe(false);
+  });
+
+  it('rejects a host-case mismatch', () => {
+    expect(isOriginAllowed('https://APP.example', allowlist)).toBe(false);
+  });
+
+  it('allows no-Origin but rejects cross-origin with an empty allowlist', () => {
+    const empty: ReadonlySet<string> = new Set<string>();
+    expect(isOriginAllowed(undefined, empty)).toBe(true);
+    expect(isOriginAllowed('https://app.example', empty)).toBe(false);
+  });
+});
+
+describe('buildCorsAllowlist', () => {
+  it('contains the app URL, each configured origin, and all WebView origins', () => {
+    const allowlist = buildCorsAllowlist({
+      appUrl: 'https://app.example',
+      configuredOrigins: ['https://a.example', 'https://b.example'],
+    });
+
+    expect(allowlist.has('https://app.example')).toBe(true);
+    expect(allowlist.has('https://a.example')).toBe(true);
+    expect(allowlist.has('https://b.example')).toBe(true);
+    for (const origin of WEBVIEW_ORIGINS) {
+      expect(allowlist.has(origin)).toBe(true);
+    }
+  });
+
+  it('deduplicates when a configured origin coincides with the app URL', () => {
+    const allowlist = buildCorsAllowlist({
+      appUrl: 'https://app.example',
+      configuredOrigins: ['https://app.example'],
+    });
+
+    // app URL + 4 WebView origins, the duplicate configured origin collapses.
+    expect(allowlist.size).toBe(1 + WEBVIEW_ORIGINS.length);
+  });
+
+  it('always includes the four WebView origins even with no configured origins', () => {
+    const allowlist = buildCorsAllowlist({
+      appUrl: 'https://app.example',
+      configuredOrigins: [],
+    });
+
+    for (const origin of WEBVIEW_ORIGINS) {
+      expect(allowlist.has(origin)).toBe(true);
+    }
+  });
+});

apps/server/src/integrations/environment/cors.util.ts

@@ -0,0 +1,39 @@
+// CORS trust boundary helpers. `buildCorsAllowlist` produces the exact set of
+// origins the API trusts, and `isOriginAllowed` is the predicate the enableCors
+// origin callback uses to accept/reject each request. With credentials:true a
+// foreign credentialed origin must never be allowed, so anything not in the
+// allowlist (apart from no-Origin requests) is rejected.
+
+// Native WebView origins used by the Capacitor/Ionic mobile shell. Always
+// trusted so the native client can call the API. CORS hardening of these is
+// intentionally out of scope.
+const NATIVE_WEBVIEW_ORIGINS = [
+  'capacitor://localhost',
+  'ionic://localhost',
+  'http://localhost',
+  'https://localhost',
+] as const;
+
+// Build the CORS allowlist: the app URL, all configured cross-origin clients,
+// and the native WebView origins. Dedup is automatic via Set.
+export function buildCorsAllowlist(input: {
+  appUrl: string;
+  configuredOrigins: readonly string[];
+}): Set<string> {
+  return new Set<string>([
+    input.appUrl,
+    ...input.configuredOrigins,
+    ...NATIVE_WEBVIEW_ORIGINS,
+  ]);
+}
+
+// Decide whether a request's Origin is allowed. A missing Origin header (curl,
+// server-to-server, some native WebViews) is allowed; otherwise the origin must
+// be present in the allowlist.
+export function isOriginAllowed(
+  origin: string | undefined,
+  allowlist: ReadonlySet<string>,
+): boolean {
+  if (!origin) return true;
+  return allowlist.has(origin);
+}

apps/server/src/main.ts

@@ -15,6 +15,10 @@ import { InternalLogFilter } from './common/logger/internal-log-filter';
 import { EnvironmentService } from './integrations/environment/environment.service';
 import { resolveFrameHeader } from './common/helpers';
 import { resolveTrustProxy } from './integrations/environment/trust-proxy.util';
+import {
+  buildCorsAllowlist,
+  isOriginAllowed,
+} from './integrations/environment/cors.util';
 import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger';
 
 async function bootstrap() {
@@ -147,25 +151,19 @@ async function bootstrap() {
   // The web client is same-origin in production; an explicit allowlist lets
   // native/mobile WebView origins (Capacitor) and any configured cross-origin
   // clients call the API, while everything else is rejected.
-  const corsAllowedOrigins = new Set<string>([
-    environmentService.getAppUrl(),
-    ...environmentService.getCorsAllowedOrigins(),
-    // Capacitor / Ionic WebView origins used by the native shell.
-    'capacitor://localhost',
-    'ionic://localhost',
-    'http://localhost',
-    'https://localhost',
-  ]);
+  const corsAllowedOrigins = buildCorsAllowlist({
+    appUrl: environmentService.getAppUrl(),
+    configuredOrigins: environmentService.getCorsAllowedOrigins(),
+  });
 
   app.enableCors({
     // Allow requests with no Origin header (curl, server-to-server, some native
     // WebView requests) and any origin in the allowlist; reject the rest.
-    origin: (origin: string | undefined, callback: (err: Error | null, allow?: boolean) => void) => {
-      if (!origin || corsAllowedOrigins.has(origin)) {
-        callback(null, true);
-        return;
-      }
-      callback(null, false);
+    origin: (
+      origin: string | undefined,
+      callback: (err: Error | null, allow?: boolean) => void,
+    ) => {
+      callback(null, isOriginAllowed(origin, corsAllowedOrigins));
     },
     credentials: true,
     methods: ['GET', 'HEAD', 'PUT', 'PATCH', 'POST', 'DELETE', 'OPTIONS'],