fix(sync): report honest status when SuperSync encryption key is missing (#8741)

* fix(work-context): use project icon for header title icon

The header title icon read the active work context's icon, but
selectActiveWorkContext forced icon: null for projects (a leftover
from when projects had no icon field). Pass the project's own icon
through so the header matches the side nav.

* fix(sync): report honest status when SuperSync encryption key is missing

When a provider mandates E2E encryption (SuperSync) but no key is
configured, the GHSA-9v8x guard skips the upload while pending ops stay
unsynced. The low-level result was byte-identical to "nothing to upload",
so the wrapper claimed IN_SYNC — silently one-way syncing (downloads
apply, local edits never leave; device loss = data loss).

Thread an `encryptionRequiredKeyMissing` flag from the upload guard
through UploadResult and the orchestrator's UploadOutcome so the wrapper
reports UNKNOWN_OR_CHANGED (UpdateRemote) instead of IN_SYNC, and
short-circuits the LWW re-upload loop that would otherwise spin against
the same missing key.

Also stop reverting to a keyless config when enable-encryption fails
after the server was already wiped: that left a mandatory-encryption
provider with no key, so the upload guard blocked every future upload and
the wiped server could never be repopulated (the advertised "Sync Now"
recovery was a no-op — account stranded). Keep the new key so the next
sync re-uploads the data encrypted, mirroring the password-change flow.

Refs #8731
This commit is contained in:
Johannes Millan 2026-07-03 18:35:33 +02:00 committed by GitHub
parent bf710637d2
commit cb586f87e9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 94 additions and 25 deletions

View file

@ -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 () => {

View file

@ -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 },
);

View file

@ -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({

View file

@ -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;

View file

@ -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. */

View file

@ -376,6 +376,9 @@ export class OperationLogSyncService {
permanentRejectionCount: rejectionResult.permanentRejectionCount,
hasMorePiggyback: result.hasMorePiggyback ?? false,
rejectedOps: result.rejectedOps,
...(result.encryptionRequiredKeyMissing
? { encryptionRequiredKeyMissing: true }
: {}),
};
}

View file

@ -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,
});
});

View file

@ -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 } : {}),
};
}