From 8d7e05ccebfa3a824468f1d719b011cd5e4eab51 Mon Sep 17 00:00:00 2001 From: johannesjo Date: Wed, 13 May 2026 00:45:41 +0200 Subject: [PATCH] fix(supersync): plug residual gaps surfaced by post-slice review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two post-merge findings from the careful review pass on the SuperSync slice (PR 7b): 1. **Storage NaN bug.** The app-side `localStorage`-backed `SuperSyncStorage.getLastServerSeq` returned `NaN` for an empty- string value (`parseInt("", 10) === NaN`), which the package's `stored ?? 0` fallback could not catch (nullish coalescing only triggers on `null`/`undefined`). The original implementation used `stored ? parseInt(stored, 10) : 0` and returned `0` for any falsy string. Add an explicit `Number.isFinite` check in the adapter so the port contract (returns `number | null`) is actually honored. Doesn't happen in practice today — `setItem` always writes a stringified int — but a hypothetical regression that wrote `""` would silently break sync replay. 2. **`_handleNativeRequestError` residual hostname leak.** The non-retryable native path re-threw the original error unchanged. Foreign errors from the native HTTP executor (e.g. iOS TLS cert errors like "SSL certificate problem: hostname mismatch for sync.example.com") can embed the resolved hostname in `.message`, which surfaces in user-visible snack notifications via `super-sync-restore.service.ts` and similar. Scrub to a name-only form (`SuperSync native request failed: `) while preserving identity for our own thrown errors (`AuthFailSPError`, `MissingCredentialsSPError`, `HTTP ` form). The full message is still captured in the structured log meta for diagnostics. Two new Vitest specs cover the scrub path and the `AuthFailSPError` identity preservation through the non-retryable native branch. Package spec count: 275 (was 273). --- .../src/super-sync/super-sync.ts | 17 +++++++ .../tests/super-sync/super-sync.spec.ts | 45 +++++++++++++++++++ .../sync-providers/super-sync/super-sync.ts | 7 ++- 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/packages/sync-providers/src/super-sync/super-sync.ts b/packages/sync-providers/src/super-sync/super-sync.ts index e1f7bd4844..d74e134bb2 100644 --- a/packages/sync-providers/src/super-sync/super-sync.ts +++ b/packages/sync-providers/src/super-sync/super-sync.ts @@ -538,6 +538,23 @@ export class SuperSyncProvider 'Unable to connect to SuperSync server. Check your internet connection.', ); } + // Our own thrown errors (`AuthFailSPError`, `MissingCredentialsSPError`, + // `HTTP ` from the non-2xx branch) carry only scrubbed content + // and propagate unchanged. Foreign errors from the native HTTP executor + // (e.g. iOS TLS-cert errors like "Hostname mismatch for example.com") + // can embed the resolved hostname in `.message`. Replace those with a + // name-only surface — the full message is captured above via the + // structured logger. + if ( + error instanceof AuthFailSPError || + error instanceof MissingCredentialsSPError || + (error instanceof Error && error.message.startsWith('HTTP ')) + ) { + throw error; + } + if (error instanceof Error) { + throw new Error(`SuperSync native request failed: ${error.name}`); + } throw error; } diff --git a/packages/sync-providers/tests/super-sync/super-sync.spec.ts b/packages/sync-providers/tests/super-sync/super-sync.spec.ts index 8ff15b5e42..128eeb2def 100644 --- a/packages/sync-providers/tests/super-sync/super-sync.spec.ts +++ b/packages/sync-providers/tests/super-sync/super-sync.spec.ts @@ -1339,6 +1339,51 @@ describe('SuperSyncProvider', () => { // Privacy: the URL/hostname must NOT be in the user-facing message. expect(caught!.message).not.toContain('internal-host.invalid'); }); + + /** + * Non-retryable foreign native errors (e.g. iOS TLS-cert errors) can + * embed the resolved hostname in `.message`. Verify the user-facing + * surface scrubs to a name-only form so dialogs / snacks don't leak. + */ + it('scrubs non-retryable foreign native errors (e.g. TLS cert) to name-only surface', async () => { + const ctx = buildProvider({ isNativePlatform: true }); + ctx.cfgStore.load.mockResolvedValue(testConfig); + const tlsError = new Error( + 'SSL certificate problem: hostname mismatch for sync.example.com', + ); + tlsError.name = 'TlsError'; + ctx.nativeHttpExecutor.mockRejectedValue(tlsError); + + let caught: Error | undefined; + try { + await ctx.provider.uploadOps([createMockOperation()], 'client-1'); + } catch (e) { + caught = e as Error; + } + expect(caught?.message).toBe('SuperSync native request failed: TlsError'); + // Privacy: the hostname from the cert error must NOT leak. + expect(caught!.message).not.toContain('sync.example.com'); + expect(caught!.message).not.toContain('hostname mismatch'); + }); + + /** + * Our own thrown errors carry only scrubbed content (HTTP , + * AuthFailSPError, MissingCredentialsSPError) and must propagate + * unchanged on the non-retryable native path. + */ + it('preserves AuthFailSPError identity through non-retryable native path', async () => { + const ctx = buildProvider({ isNativePlatform: true }); + ctx.cfgStore.load.mockResolvedValue(testConfig); + ctx.nativeHttpExecutor.mockResolvedValue({ + status: 401, + headers: {}, + data: { error: 'token expired' }, + }); + + await expect( + ctx.provider.uploadOps([createMockOperation()], 'client-1'), + ).rejects.toBeInstanceOf(AuthFailSPError); + }); }); describe('Request timeout handling', () => { diff --git a/src/app/op-log/sync-providers/super-sync/super-sync.ts b/src/app/op-log/sync-providers/super-sync/super-sync.ts index a5827e6904..55d09349c3 100644 --- a/src/app/op-log/sync-providers/super-sync/super-sync.ts +++ b/src/app/op-log/sync-providers/super-sync/super-sync.ts @@ -41,7 +41,12 @@ export const createSuperSyncProvider = (): PackageSuperSyncProvider => { const localStoragePort: SuperSyncDeps['storage'] = { getLastServerSeq: (key) => { const v = localStorage.getItem(key); - return v == null ? null : Number.parseInt(v, 10); + if (v == null || v === '') return null; + // `parseInt` returns NaN for non-numeric strings (e.g. a future + // regression that writes ""). Bridge to `null` so the package's + // `stored ?? 0` fallback fires correctly. + const n = Number.parseInt(v, 10); + return Number.isFinite(n) ? n : null; }, setLastServerSeq: (key, value) => localStorage.setItem(key, String(value)), removeLastServerSeq: (key) => localStorage.removeItem(key),