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 a48434233b..f1205bcd22 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts @@ -101,7 +101,7 @@ describe('SuperSyncEncryptionToggleService', () => { // 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 () => { + it('should NOT revert to a keyless config on failure (would strand the wiped account)', async () => { mockSnapshotUploadService.deleteAndReuploadWithNewEncryption.and.rejectWith( new Error('Upload failed'), ); @@ -110,16 +110,11 @@ describe('SuperSyncEncryptionToggleService', () => { /Failed to upload encrypted snapshot/, ); - // Should revert using the config captured BEFORE the destructive call - expect(mockProviderManager.setProviderConfig).toHaveBeenCalledWith( - SyncProviderId.SuperSync, - jasmine.objectContaining({ - baseUrl: 'https://test.example.com', - accessToken: 'test-token', - encryptKey: undefined, - isEncryptionEnabled: false, - }), - ); + // The server is already wiped and the new key persisted by + // deleteAndReuploadWithNewEncryption. Reverting to encryptKey:undefined would + // block every future upload (mandatory encryption + no key). The toggle service + // must keep the key, so it must NOT write a keyless config on failure. + expect(mockProviderManager.setProviderConfig).not.toHaveBeenCalled(); }); it('should throw with descriptive message including reason 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 222df04118..4c62567d7a 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.ts @@ -73,22 +73,24 @@ export class SuperSyncEncryptionToggleService { SyncLog.normal(`${LOG_PREFIX}: Encryption enabled successfully!`); } catch (error) { - // Revert config on failure (server data is already deleted at this point) - SyncLog.err(`${LOG_PREFIX}: Failed after deleting server data!`, error); - - // Best-effort revert: restore pre-enable config. - // existingCfg was captured before enabling, so it already has isEncryptionEnabled: false - // and no encryptKey. The explicit overrides ensure correctness even if the guard above - // was bypassed (e.g., existingCfg was partially set). - await this._providerManager.setProviderConfig(SyncProviderId.SuperSync, { - ...existingCfg, - encryptKey: undefined, - isEncryptionEnabled: false, - } as SuperSyncPrivateCfg); + // IMPORTANT: Do NOT revert to a keyless config here. + // deleteAndReuploadWithNewEncryption deletes all server data and persists the new + // key config BEFORE the upload that just failed, so the server is now empty. Reverting + // to `encryptKey: undefined` would leave a mandatory-encryption provider with no key — + // the op-log upload guard then skips EVERY future upload, so the wiped server can never + // be repopulated and the advertised "Sync Now" recovery is a no-op (account stranded). + // Keep the new key (as the password-change flow does) so the next sync re-uploads the + // local data, encrypted. If the failure happened before the config was persisted, the + // config is simply still the original — nothing to revert either way. + SyncLog.err( + `${LOG_PREFIX}: Upload failed after deleting server data — keeping new encryption key for retry`, + error, + ); throw new Error( 'Failed to upload encrypted snapshot after deleting server data. ' + - 'Your local data is safe. Encryption has been reverted. Please use "Sync Now" to re-upload your data. ' + + 'Your local data is safe and encryption stays enabled. ' + + 'Your data will re-upload (encrypted) on the next sync — or click "Sync Now" to retry now. ' + `Reason: ${_friendlyErrorMessage(error)}`, { cause: error }, ); diff --git a/src/app/imex/sync/sync-wrapper.service.spec.ts b/src/app/imex/sync/sync-wrapper.service.spec.ts index ad8e1fdfd3..578d014d5e 100644 --- a/src/app/imex/sync/sync-wrapper.service.spec.ts +++ b/src/app/imex/sync/sync-wrapper.service.spec.ts @@ -314,6 +314,33 @@ describe('SyncWrapperService', () => { expect(result).toBe(SyncStatus.UpdateRemote); }); + // #8731: an encryption-mandatory provider with no key skips upload (GHSA-9v8x + // guard) while pending ops remain unsynced. This must NOT be reported as IN_SYNC. + it('should report UNKNOWN_OR_CHANGED (not InSync) when upload was skipped for a missing mandatory key', async () => { + mockSyncService.downloadRemoteOps.and.returnValue( + Promise.resolve({ kind: 'no_new_ops' as const }), + ); + mockSyncService.uploadPendingOps.and.returnValue( + Promise.resolve({ + kind: 'completed' as const, + uploadedCount: 0, + piggybackedOpsCount: 0, + localWinOpsCreated: 0, + permanentRejectionCount: 0, + hasMorePiggyback: false, + rejectedOps: [], + encryptionRequiredKeyMissing: true, + }), + ); + + const result = await service.sync(); + + expect(result).toBe(SyncStatus.UpdateRemote); + expect(mockProviderManager.setSyncStatus).toHaveBeenCalledWith( + 'UNKNOWN_OR_CHANGED', + ); + }); + it('should return UpdateRemote when remote ops were downloaded', async () => { mockSyncService.downloadRemoteOps.and.returnValue( Promise.resolve({ diff --git a/src/app/imex/sync/sync-wrapper.service.ts b/src/app/imex/sync/sync-wrapper.service.ts index 41f5300282..5bbd7d301f 100644 --- a/src/app/imex/sync/sync-wrapper.service.ts +++ b/src/app/imex/sync/sync-wrapper.service.ts @@ -531,6 +531,24 @@ export class SyncWrapperService { return 'HANDLED_ERROR'; } + // If the provider mandates encryption but no key is configured yet, the upload was + // skipped with pending ops still unsynced (GHSA-9v8x guard). Downloads still ran + // (merge-first), but nothing local can be uploaded until encryption is set up — so + // this is NOT in sync. Report it honestly instead of falling through to IN_SYNC. + // Also short-circuit the LWW loop below: its local-win ops are blocked by the same + // missing key and would just spin to the retry cap. + if ( + uploadResult.kind === 'completed' && + uploadResult.encryptionRequiredKeyMissing + ) { + SyncLog.log( + 'SyncWrapperService: Upload skipped — encryption required but no key configured. ' + + 'Reporting UNKNOWN_OR_CHANGED (sync paused until encryption is set up).', + ); + this._providerManager.setSyncStatus('UNKNOWN_OR_CHANGED'); + return SyncStatus.UpdateRemote; + } + // 3. If LWW created local-win ops, upload them (with retry limit to prevent infinite loops) const downloadLwwOps = downloadResult.kind === 'ops_processed' ? downloadResult.localWinOpsCreated : 0; diff --git a/src/app/op-log/core/types/sync-results.types.ts b/src/app/op-log/core/types/sync-results.types.ts index af3ed0e42c..f684fe756b 100644 --- a/src/app/op-log/core/types/sync-results.types.ts +++ b/src/app/op-log/core/types/sync-results.types.ts @@ -151,6 +151,14 @@ export interface UploadResult { * or the next download skips them forever. (#8304) */ lastServerSeqToPersist?: number; + /** + * True when the provider mandates E2E encryption (SuperSync) but no key is configured + * yet, so the GHSA-9v8x guard skipped the upload while pending ops remain unsynced. + * Lets the caller report an honest "not in sync — encryption required" status instead + * of claiming IN_SYNC after what looks like a zero-op upload. Only set when there were + * pending ops to upload (the guard fires after the empty-ops check). + */ + encryptionRequiredKeyMissing?: boolean; } /** @@ -267,6 +275,11 @@ export type UploadOutcome = permanentRejectionCount: number; hasMorePiggyback: boolean; rejectedOps: RejectedOpInfo[]; + /** + * True when the upload was skipped because the provider mandates encryption but no + * key is configured, leaving pending ops unsynced. The wrapper must not claim IN_SYNC. + */ + encryptionRequiredKeyMissing?: boolean; } | { /** User cancelled a piggybacked SYNC_IMPORT conflict dialog. */ diff --git a/src/app/op-log/sync/operation-log-sync.service.ts b/src/app/op-log/sync/operation-log-sync.service.ts index eccfb8d473..48056865a1 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -376,6 +376,9 @@ export class OperationLogSyncService { permanentRejectionCount: rejectionResult.permanentRejectionCount, hasMorePiggyback: result.hasMorePiggyback ?? false, rejectedOps: result.rejectedOps, + ...(result.encryptionRequiredKeyMissing + ? { encryptionRequiredKeyMissing: true } + : {}), }; } diff --git a/src/app/op-log/sync/operation-log-upload.service.spec.ts b/src/app/op-log/sync/operation-log-upload.service.spec.ts index ae101200da..fd7ba4d029 100644 --- a/src/app/op-log/sync/operation-log-upload.service.spec.ts +++ b/src/app/op-log/sync/operation-log-upload.service.spec.ts @@ -163,14 +163,17 @@ describe('OperationLogUploadService', () => { expect(mockOpLogStore.markSynced).not.toHaveBeenCalled(); }); - it('returns an empty upload result', async () => { + it('returns a result flagged encryptionRequiredKeyMissing (so the caller does not claim IN_SYNC)', async () => { const result = await service.uploadPendingOps(mockApiProvider); + // Pending ops remained unsynced. The flag distinguishes this from a genuine + // "nothing to upload" so the wrapper reports an honest not-in-sync status. expect(result).toEqual({ uploadedCount: 0, rejectedCount: 0, piggybackedOps: [], rejectedOps: [], + encryptionRequiredKeyMissing: true, }); }); diff --git a/src/app/op-log/sync/operation-log-upload.service.ts b/src/app/op-log/sync/operation-log-upload.service.ts index 7f1b04ffcc..c403eb0a86 100644 --- a/src/app/op-log/sync/operation-log-upload.service.ts +++ b/src/app/op-log/sync/operation-log-upload.service.ts @@ -92,6 +92,9 @@ export class OperationLogUploadService { // last-server-seq covering them must be persisted by the caller AFTER those ops are // applied — not here. We surface the planned value instead of persisting it. let lastServerSeqToPersist: number | undefined; + // Set when the mandatory-encryption guard skips the upload with pending ops still + // unsynced, so the caller can report an honest not-in-sync status (not IN_SYNC). + let encryptionRequiredKeyMissing = false; await this.lockService.request(LOCK_NAMES.UPLOAD, async () => { // Execute pre-upload callback INSIDE the lock, BEFORE checking for pending ops. @@ -132,6 +135,10 @@ export class OperationLogUploadService { // upload. Without this guard the initial setup sync pushes all local ops to // the server in cleartext, breaking the E2EE promise even if later deleted. if (syncProvider.isEncryptionMandatory && !encryptKey) { + // We got past the empty-ops check above, so there ARE pending ops we are + // refusing to upload. Flag it so the caller reports an honest not-in-sync + // status instead of IN_SYNC (which a plain uploadedCount:0 would look like). + encryptionRequiredKeyMissing = true; // Expected during the pre-encryption setup window (fires on every // auto-sync until a key is set), so log at normal level, not warn. OpLog.normal( @@ -433,6 +440,7 @@ export class OperationLogUploadService { ...(hasMorePiggyback ? { hasMorePiggyback: true } : {}), ...(piggybackHasOnlyUnencryptedData ? { piggybackHasOnlyUnencryptedData } : {}), ...(lastServerSeqToPersist !== undefined ? { lastServerSeqToPersist } : {}), + ...(encryptionRequiredKeyMissing ? { encryptionRequiredKeyMissing: true } : {}), }; }