From 87f092bee3b993c05203189f8faaa46e98c0467d Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 13 May 2026 19:49:06 +0200 Subject: [PATCH] fix(sync): correct error class names, JSDoc, and missed cache-clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from a multi-agent review of the recent sync extraction: - Seven error classes in @sp/sync-providers shipped with a leading space in `name`, and `UploadRevToMatchMismatchAPIError` was further truncated to ' UploadRevToMatchMismatchAP'. Consumers use instanceof so runtime behavior was preserved, but stack traces, error envelopes, and structured-log meta carried the broken names. Added a regression test asserting instance.name matches ErrCtor.name for all 14 classes. - @sp/sync-core decryptBatch JSDoc claimed Argon2 errors are never silently masked as legacy fallbacks, contradicting the actual catch-and-decryptLegacy path. The fallback is part of the public wire-format contract; rewrote the comment to match the implementation and reference the module-level wire-format spec. - SuperSyncEncryptionToggleService.enable/disableEncryption promised "Clear cache on success" but never called clearSessionKeyCache(). Added the call on the success path in both methods, matching the pattern used by EncryptionPasswordChangeService and FileBasedEncryptionService. - Removed packages/sync-core/tests/ports.spec.ts — 57 LOC of vi.fn().mockResolvedValue type-assertion tests with no production code under test. Port shapes are enforced at the host via `implements` clauses with real behavioral coverage in sibling specs. --- packages/sync-core/src/encryption.ts | 9 ++- packages/sync-core/tests/ports.spec.ts | 57 ------------------- packages/sync-providers/src/errors/index.ts | 14 ++--- packages/sync-providers/tests/errors.spec.ts | 29 ++++++++++ ...upersync-encryption-toggle.service.spec.ts | 6 ++ .../supersync-encryption-toggle.service.ts | 8 +++ 6 files changed, 56 insertions(+), 67 deletions(-) delete mode 100644 packages/sync-core/tests/ports.spec.ts diff --git a/packages/sync-core/src/encryption.ts b/packages/sync-core/src/encryption.ts index 26a8995e86..f058a6208e 100644 --- a/packages/sync-core/src/encryption.ts +++ b/packages/sync-core/src/encryption.ts @@ -263,9 +263,12 @@ export const encryptBatch = async ( * Operations with the same salt (e.g., encrypted in the same batch) share * the cached key, avoiding redundant Argon2id derivations. * - * SECURITY: Unlike single-item decrypt(), uses explicit format detection so - * Argon2 decryption errors are not silently masked as legacy fallbacks. - * Only ciphertexts too short for Argon2 attempt legacy decryption. + * Format handling mirrors single-item `decrypt()`: ciphertexts in the + * legacy-length range (28..43 bytes) take the PBKDF2 path; >= 44 bytes are + * attempted as Argon2id first, with a legacy fallback on auth failure (a + * long legacy ciphertext can be misclassified as Argon2 by the length + * heuristic); < 28 bytes throws as invalid. The fallback is part of the + * public wire-format contract; see the module-level JSDoc. */ export const decryptBatch = async ( dataItems: string[], diff --git a/packages/sync-core/tests/ports.spec.ts b/packages/sync-core/tests/ports.spec.ts deleted file mode 100644 index 01f07ebee9..0000000000 --- a/packages/sync-core/tests/ports.spec.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { describe, expect, it, vi } from 'vitest'; -import type { ConflictUiPort, SyncConfigPort, SyncConfigSnapshot } from '../src'; - -describe('sync-core ports', () => { - it('keeps sync config provider IDs as host-owned strings', async () => { - type ProviderId = 'super-sync' | 'file-sync'; - const config: SyncConfigSnapshot = { - isEnabled: true, - syncProvider: 'super-sync', - isEncryptionEnabled: true, - isCompressionEnabled: false, - isManualSyncOnly: false, - syncInterval: 5, - }; - const port: SyncConfigPort = { - getSyncConfig: vi.fn().mockResolvedValue(config), - }; - - await expect(port.getSyncConfig()).resolves.toBe(config); - expect(port.getSyncConfig).toHaveBeenCalledOnce(); - }); - - it('keeps conflict UI reasons and resolutions host-owned strings', async () => { - type Resolution = 'USE_LOCAL' | 'USE_REMOTE' | 'CANCEL'; - const notify = vi.fn(); - const port: ConflictUiPort = { - showConflictDialog: vi.fn().mockResolvedValue('USE_LOCAL'), - notify, - }; - - await expect( - port.showConflictDialog({ - conflictType: 'sync-import', - scenario: 'LOCAL_IMPORT_FILTERS_REMOTE', - reason: 'BACKUP_RESTORE', - counts: { filteredOpCount: 3 }, - timestamps: { localImportTimestamp: 123 }, - meta: { providerId: 'super-sync' }, - }), - ).resolves.toBe('USE_LOCAL'); - - port.notify?.({ - severity: 'warning', - message: 'sync-conflict', - reason: 'BACKUP_RESTORE', - meta: { filteredOps: 3 }, - }); - - expect(port.showConflictDialog).toHaveBeenCalledOnce(); - expect(notify).toHaveBeenCalledWith({ - severity: 'warning', - message: 'sync-conflict', - reason: 'BACKUP_RESTORE', - meta: { filteredOps: 3 }, - }); - }); -}); diff --git a/packages/sync-providers/src/errors/index.ts b/packages/sync-providers/src/errors/index.ts index cd565370f1..fe24024600 100644 --- a/packages/sync-providers/src/errors/index.ts +++ b/packages/sync-providers/src/errors/index.ts @@ -39,11 +39,11 @@ export class AdditionalLogErrorBase extends Error { // --------------API ERRORS-------------- export class NoRevAPIError extends AdditionalLogErrorBase { - override name = ' NoRevAPIError'; + override name = 'NoRevAPIError'; } export class FileHashCreationAPIError extends AdditionalLogErrorBase { - override name = ' FileHashCreationAPIError'; + override name = 'FileHashCreationAPIError'; } export class TooManyRequestsAPIError extends AdditionalLogErrorBase<{ @@ -51,7 +51,7 @@ export class TooManyRequestsAPIError extends AdditionalLogErrorBase<{ retryAfter?: number; path?: string; }> { - override name = ' TooManyRequestsAPIError'; + override name = 'TooManyRequestsAPIError'; readonly status: number; readonly retryAfter?: number; readonly path?: string; @@ -66,19 +66,19 @@ export class TooManyRequestsAPIError extends AdditionalLogErrorBase<{ } export class RemoteFileNotFoundAPIError extends AdditionalLogErrorBase { - override name = ' RemoteFileNotFoundAPIError'; + override name = 'RemoteFileNotFoundAPIError'; } export class MissingRefreshTokenAPIError extends Error { - override name = ' MissingRefreshTokenAPIError'; + override name = 'MissingRefreshTokenAPIError'; } export class UploadRevToMatchMismatchAPIError extends AdditionalLogErrorBase { - override name = ' UploadRevToMatchMismatchAP'; + override name = 'UploadRevToMatchMismatchAPIError'; } export class HttpNotOkAPIError extends AdditionalLogErrorBase { - override name = ' HttpNotOkAPIError'; + override name = 'HttpNotOkAPIError'; /** * Raw `Response` object retained so callers can read `.status` / * `.statusText`. diff --git a/packages/sync-providers/tests/errors.spec.ts b/packages/sync-providers/tests/errors.spec.ts index 04f35e9cd2..2186737c39 100644 --- a/packages/sync-providers/tests/errors.spec.ts +++ b/packages/sync-providers/tests/errors.spec.ts @@ -9,6 +9,7 @@ import { InvalidDataSPError, MissingCredentialsSPError, MissingRefreshTokenAPIError, + NetworkUnavailableSPError, NoRevAPIError, PotentialCorsError, RemoteFileChangedUnexpectedly, @@ -171,6 +172,7 @@ describe('Error class identity (single definition per class)', () => { ['UploadRevToMatchMismatchAPIError', UploadRevToMatchMismatchAPIError], ['PotentialCorsError', PotentialCorsError], ['RemoteFileChangedUnexpectedly', RemoteFileChangedUnexpectedly], + ['NetworkUnavailableSPError', NetworkUnavailableSPError], ]; it.each(ERROR_CLASSES)('%s is an Error subclass', (_name, ErrCtor) => { @@ -178,6 +180,33 @@ describe('Error class identity (single definition per class)', () => { expect(Object.create(ErrCtor.prototype) instanceof Error).toBe(true); }); + // Regression guard: prior versions shipped seven classes with a leading + // space in `name` (' NoRevAPIError') and one truncated to + // ' UploadRevToMatchMismatchAP'. Equality with the class identifier keeps + // the string identity log-correct and matches Error subclass convention. + // Asserting against `ErrCtor.name` (the static class identifier, which TS + // cannot synthesize with a leading space) rather than the tuple string + // means a future "fix" that co-mutates the test fixture and the source + // cannot mask a regression. + // `name` is an instance override (not a prototype property), so we must + // instantiate. Each class has a different constructor signature; pass a + // minimal stub that exercises the field-only branches. + it.each(ERROR_CLASSES)('%s instance.name matches the class name', (_name, ErrCtor) => { + let instance: Error; + if (ErrCtor === HttpNotOkAPIError) { + instance = new HttpNotOkAPIError( + new Response(null, { status: 500, statusText: 'Internal Server Error' }), + ); + } else if (ErrCtor === TooManyRequestsAPIError) { + instance = new TooManyRequestsAPIError({ status: 429 }); + } else if (ErrCtor === PotentialCorsError) { + instance = new PotentialCorsError('https://example.test'); + } else { + instance = new (ErrCtor as new (...a: never[]) => Error)(); + } + expect(instance.name).toBe(ErrCtor.name); + }); + it('EmptyRemoteBodySPError extends InvalidDataSPError', () => { const err = new EmptyRemoteBodySPError('empty body'); expect(err).toBeInstanceOf(InvalidDataSPError); diff --git a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts index ee78f01339..a48434233b 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts @@ -96,6 +96,9 @@ describe('SuperSyncEncryptionToggleService', () => { isEncryptionEnabled: true, logPrefix: 'SuperSyncEncryptionToggleService', }); + // clearSessionKeyCache() is called directly on success (module-level + // function, not spyable). Matches the pattern in + // file-based-encryption.service.ts and encryption-password-change.service.ts. }); it('should revert config on failure using pre-captured config (preserving auth credentials)', async () => { @@ -157,6 +160,9 @@ describe('SuperSyncEncryptionToggleService', () => { isEncryptionEnabled: false, logPrefix: 'SuperSyncEncryptionToggleService', }); + // clearSessionKeyCache() is called directly on success (module-level + // function, not spyable). Matches the pattern in + // file-based-encryption.service.ts and encryption-password-change.service.ts. }); it('should revert config to original state (including encryption key) on failure', async () => { diff --git a/src/app/imex/sync/supersync-encryption-toggle.service.ts b/src/app/imex/sync/supersync-encryption-toggle.service.ts index 2ef27d4d6c..222df04118 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.ts @@ -1,5 +1,6 @@ import { inject, Injectable } from '@angular/core'; import type { SuperSyncPrivateCfg } from '@sp/sync-providers/super-sync'; +import { clearSessionKeyCache } from '@sp/sync-core'; import { SyncLog } from '../../core/log'; import { SnapshotUploadService } from './snapshot-upload.service'; import { SyncProviderManager } from '../../op-log/sync-providers/provider-manager.service'; @@ -66,6 +67,10 @@ export class SuperSyncEncryptionToggleService { logPrefix: LOG_PREFIX, }); + // Drop any cached derived key from the pre-toggle password; subsequent + // sync cycles must re-derive against the new `encryptKey`. + clearSessionKeyCache(); + SyncLog.normal(`${LOG_PREFIX}: Encryption enabled successfully!`); } catch (error) { // Revert config on failure (server data is already deleted at this point) @@ -109,6 +114,9 @@ export class SuperSyncEncryptionToggleService { logPrefix: LOG_PREFIX, }); + // Drop any cached derived key now that encryption is off. + clearSessionKeyCache(); + SyncLog.normal(`${LOG_PREFIX}: Encryption disabled successfully!`); } catch (uploadError) { SyncLog.err(