From ae4d9d71043ef3b82ed37e7b1650f5edbc0f8052 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 29 Jun 2026 15:38:08 +0200 Subject: [PATCH] fix(sync): verify file-based upload size to catch truncated writes (#8604) (#8628) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): verify file-based upload size to catch truncated writes (#8604) Dropbox and OneDrive enforce no end-to-end integrity, so a truncated upload (a cut-short body silently accepted) lands a partial gzip/JSON that fails to decode on every later download until the file is deleted. The #7300 post-upload verification was WebDAV-only. Add a shared assertUploadedSizeMatches() helper: compare the stored byte size (already in the upload response, no extra request) against the bytes sent, but only for pure-ASCII payloads so the comparison is transport-encoding independent — avoiding false positives on the native CapacitorHttp path, where a wrong byte-count assumption would silently re-upload every cycle. Compressed/encrypted payloads are base64 (ASCII), covering the reported case. On mismatch raise UploadRevToMatchMismatchAPIError, the error the upload path already surfaces. This detects and fails the sync loudly; it does not repair the remote. Wired into Dropbox and OneDrive uploads. WebDAV keeps its stronger content-hash re-read check. * perf(sync): detect ASCII without encoding in upload-size check (#8604) The upload-size guard called TextEncoder.encode(data).length purely to detect whether the payload is pure-ASCII (byte count === char count), allocating a full multi-MB Uint8Array on every Dropbox/OneDrive upload — including the non-ASCII skip path, which is the default-config common case. Detect non-ASCII with a regex instead and use data.length directly; behaviour is identical (byteLen === data.length iff pure ASCII) with no allocation and an early exit on the first non-ASCII unit. Also add OneDrive upload-size wiring tests (match + ASCII truncation), which the app-side spec previously exercised only on the skip path. Surfaced by a second multi-agent review. --- .../src/file-based/dropbox/dropbox-api.ts | 7 +++ .../src/file-based/onedrive/onedrive.ts | 7 ++- .../src/file-based/verify-upload-size.ts | 57 +++++++++++++++++++ .../file-based/dropbox/dropbox-api.spec.ts | 48 ++++++++++++++++ .../file-based/verify-upload-size.spec.ts | 52 +++++++++++++++++ .../file-based/onedrive/onedrive.spec.ts | 43 ++++++++++++++ 6 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 packages/sync-providers/src/file-based/verify-upload-size.ts create mode 100644 packages/sync-providers/tests/file-based/verify-upload-size.spec.ts diff --git a/packages/sync-providers/src/file-based/dropbox/dropbox-api.ts b/packages/sync-providers/src/file-based/dropbox/dropbox-api.ts index 8e133e7051..2bde2b5779 100644 --- a/packages/sync-providers/src/file-based/dropbox/dropbox-api.ts +++ b/packages/sync-providers/src/file-based/dropbox/dropbox-api.ts @@ -13,6 +13,7 @@ import { TooManyRequestsAPIError, UploadRevToMatchMismatchAPIError, } from '../../errors'; +import { assertUploadedSizeMatches } from '../verify-upload-size'; import { executeNativeRequestWithRetry } from '../../http/native-http-retry'; import type { NativeHttpResponse } from '../../http/native-http-retry'; import type { DropboxDeps, DropboxPrivateCfg } from './dropbox'; @@ -270,6 +271,12 @@ export class DropboxApi { throw new NoRevAPIError(); } + // Fail loudly on a truncated/partial write instead of silently storing a + // corrupt sync file (#8604). See assertUploadedSizeMatches. + if (typeof data === 'string') { + assertUploadedSizeMatches(data, result.size, targetPath); + } + return result; } catch (e) { this._deps.logger.critical( diff --git a/packages/sync-providers/src/file-based/onedrive/onedrive.ts b/packages/sync-providers/src/file-based/onedrive/onedrive.ts index ddfd04308e..542c6b6138 100644 --- a/packages/sync-providers/src/file-based/onedrive/onedrive.ts +++ b/packages/sync-providers/src/file-based/onedrive/onedrive.ts @@ -12,6 +12,7 @@ import { TooManyRequestsAPIError, UploadRevToMatchMismatchAPIError, } from '../../errors'; +import { assertUploadedSizeMatches } from '../verify-upload-size'; import { generateCodeVerifier, generateCodeChallenge } from '../../pkce'; import type { OneDrivePrivateCfg, @@ -231,10 +232,14 @@ export class OneDrive implements FileSyncProvider< headers, body: dataStr, }); - const result = (await response.json()) as { eTag?: string }; + const result = (await response.json()) as { eTag?: string; size?: number }; if (!result.eTag) { throw new NoRevAPIError('OneDrive upload missing eTag'); } + // Fail loudly on a truncated/partial write instead of silently storing a + // corrupt sync file (#8604). The Graph driveItem PUT response carries the + // stored byte size. See assertUploadedSizeMatches. + assertUploadedSizeMatches(dataStr, result.size, targetPath); return { rev: result.eTag }; } catch (e) { this._mapAndThrow(e); diff --git a/packages/sync-providers/src/file-based/verify-upload-size.ts b/packages/sync-providers/src/file-based/verify-upload-size.ts new file mode 100644 index 0000000000..7636335e2b --- /dev/null +++ b/packages/sync-providers/src/file-based/verify-upload-size.ts @@ -0,0 +1,57 @@ +import { UploadRevToMatchMismatchAPIError } from '../errors'; + +// Matches any non-ASCII UTF-16 code unit (≥ U+0080). For pure-ASCII strings the +// UTF-8 byte length equals String.length, so the on-wire byte count is known +// without encoding; for anything else it is transport-dependent. +const NON_ASCII = /[^\x00-\x7f]/; + +/** + * Detects a truncated/partial upload by comparing the byte size the remote + * reports storing against the bytes we sent. File-based providers (Dropbox, + * OneDrive) enforce no end-to-end integrity, so without this a cut-short body + * (flaky network, buffering proxy) is silently accepted and the partial + * gzip/JSON then fails to decode on every later download until the file is + * deleted (#8604, #7300). + * + * A cheaper, truncation-focused analogue of WebDAV's content-hash + * `_verifyUpload` (which re-GETs and catches any corruption). It reuses the + * size already in the upload response — no extra request — but only catches + * length-changing corruption, and only for pure-ASCII payloads (see below). It + * DETECTS and fails the sync loudly so the bad write is not recorded as synced; + * it does not repair the remote. + * + * @param data the exact string body that was uploaded + * @param storedSize byte size the provider reports storing, or undefined if the + * response omits it (then skipped — fail open) + * @param targetPath relative path, for the error message (privacy-safe) + */ +export const assertUploadedSizeMatches = ( + data: string, + storedSize: number | undefined, + targetPath: string, +): void => { + // Fail open when size is absent — never block an upload on a check we can't + // perform. In practice size ships alongside the rev/eTag the caller already + // required, so this only guards a future "minimal response" API change. + if (typeof storedSize !== 'number') { + return; + } + // Only verifiable for pure-ASCII payloads: then the stored byte count equals + // data.length on every transport (fetch UTF-8 and the native CapacitorHttp + // path alike), so a mismatch unambiguously means truncation — and no + // allocation is needed. Skip multi-byte payloads (default config ships + // compression AND encryption OFF → raw JSON with non-ASCII task content): we + // can't assume the native transport encodes byte-for-byte like TextEncoder, + // and a wrong assumption would falsely loop (re-uploading every cycle). + // Compressed/encrypted payloads are base64 (ASCII) — the #8604 case is covered. + if (NON_ASCII.test(data)) { + return; + } + if (storedSize !== data.length) { + throw new UploadRevToMatchMismatchAPIError( + `${targetPath}: remote stored ${storedSize} bytes but ${data.length} were ` + + `uploaded — the remote copy is truncated. Sync will fail until a full ` + + `copy is written.`, + ); + } +}; diff --git a/packages/sync-providers/tests/file-based/dropbox/dropbox-api.spec.ts b/packages/sync-providers/tests/file-based/dropbox/dropbox-api.spec.ts index 32febfdf3a..82243aa5bb 100644 --- a/packages/sync-providers/tests/file-based/dropbox/dropbox-api.spec.ts +++ b/packages/sync-providers/tests/file-based/dropbox/dropbox-api.spec.ts @@ -489,6 +489,54 @@ describe('DropboxApi', () => { }); }); + describe('upload integrity verification', () => { + beforeEach(() => { + const existingConfig: DropboxPrivateCfg = { + accessToken: 'test-access-token', + refreshToken: 'test-refresh-token', + encryptKey: 'test-key', + }; + (credentialStore.load as ReturnType).mockResolvedValue( + existingConfig, + ); + }); + + it('resolves when the stored byte size matches the uploaded data', async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ rev: 'new-rev', size: 4 }), + } as Response); + + const result = await dropboxApi.upload({ + path: '/test.json', + data: 'test', // 4 bytes + isForceOverwrite: true, + }); + + expect(result.rev).toBe('new-rev'); + }); + + it('throws UploadRevToMatchMismatchAPIError when an ASCII payload is truncated', async () => { + fetchSpy.mockResolvedValue({ + ok: true, + // Dropbox accepted a partial body: stored fewer bytes than we sent. + json: () => Promise.resolve({ rev: 'new-rev', size: 2 }), + } as Response); + + await expect( + dropboxApi.upload({ + path: '/test.json', + data: 'test', // 4 ASCII bytes + isForceOverwrite: true, + }), + ).rejects.toThrow(UploadRevToMatchMismatchAPIError); + }); + + // Branch logic (size absent, multi-byte skip) is covered directly in + // verify-upload-size.spec.ts; these two cases only assert the wiring — + // that upload() reads result.size and runs the check. + }); + describe('getTokensFromAuthCode', () => { it('should exchange auth code for tokens', async () => { fetchSpy.mockResolvedValue({ diff --git a/packages/sync-providers/tests/file-based/verify-upload-size.spec.ts b/packages/sync-providers/tests/file-based/verify-upload-size.spec.ts new file mode 100644 index 0000000000..f5f579f3c1 --- /dev/null +++ b/packages/sync-providers/tests/file-based/verify-upload-size.spec.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; +import { assertUploadedSizeMatches } from '../../src/file-based/verify-upload-size'; +import { UploadRevToMatchMismatchAPIError } from '../../src/errors'; + +describe('assertUploadedSizeMatches', () => { + it('passes when the stored size matches the ASCII payload byte length', () => { + expect(() => assertUploadedSizeMatches('test', 4, 'sync-data.json')).not.toThrow(); + }); + + it('throws when an ASCII payload was stored truncated (fewer bytes)', () => { + expect(() => assertUploadedSizeMatches('test', 2, 'sync-data.json')).toThrow( + UploadRevToMatchMismatchAPIError, + ); + }); + + it('throws when an ASCII payload was stored larger than sent', () => { + expect(() => assertUploadedSizeMatches('test', 9, 'sync-data.json')).toThrow( + UploadRevToMatchMismatchAPIError, + ); + }); + + it('skips (fails open) when the response omits size', () => { + expect(() => + assertUploadedSizeMatches('test', undefined, 'sync-data.json'), + ).not.toThrow(); + }); + + it('skips multi-byte payloads even when the size clearly mismatches', () => { + // 'café' = 4 UTF-16 code units but 5 UTF-8 bytes. The byte-count check is + // only transport-safe for pure-ASCII payloads, so a non-ASCII payload must + // never throw — otherwise a transport that encodes differently than + // TextEncoder would falsely loop. 2 matches neither 4 nor 5. + expect(() => assertUploadedSizeMatches('café', 2, 'sync-data.json')).not.toThrow(); + }); + + it('skips a multi-byte payload even when size equals the UTF-8 byte length', () => { + // Still skipped: we deliberately do not trust the comparison for non-ASCII. + expect(() => assertUploadedSizeMatches('café', 5, 'sync-data.json')).not.toThrow(); + }); + + it('includes the target path and both byte counts in the error', () => { + let caught: Error | undefined; + try { + assertUploadedSizeMatches('test', 2, 'sync-data.json'); + } catch (e) { + caught = e as Error; + } + expect(caught?.message).toContain('sync-data.json'); + expect(caught?.message).toContain('2'); + expect(caught?.message).toContain('4'); + }); +}); diff --git a/src/app/op-log/sync-providers/file-based/onedrive/onedrive.spec.ts b/src/app/op-log/sync-providers/file-based/onedrive/onedrive.spec.ts index a3f5d9f87a..e081a891a4 100644 --- a/src/app/op-log/sync-providers/file-based/onedrive/onedrive.spec.ts +++ b/src/app/op-log/sync-providers/file-based/onedrive/onedrive.spec.ts @@ -276,6 +276,49 @@ describe('OneDrive', () => { expect(postCount).toBe(0); }); + it('resolves an upload when the stored size matches the sent bytes (#8604)', async () => { + cfgStoreSpy.load.and.resolveTo(baseCfg); + fetchSpy.and.callFake(async (_url: string, init?: RequestInit) => { + if (init?.method === 'PUT') { + return { + ok: true, + status: 200, + // '{"a":1}' is 7 ASCII bytes — size matches, upload accepted. + json: async () => ({ eTag: 'etag-1', size: 7 }), + text: async () => '', + } as Response; + } + return { ok: true, status: 200, text: async () => '' } as Response; + }); + + await expectAsync( + provider.uploadFile('file-1.json', '{"a":1}', null, true), + ).toBeResolved(); + }); + + it('throws when OneDrive stored a truncated (smaller) ASCII payload (#8604)', async () => { + cfgStoreSpy.load.and.resolveTo(baseCfg); + fetchSpy.and.callFake(async (_url: string, init?: RequestInit) => { + if (init?.method === 'PUT') { + return { + ok: true, + status: 200, + // Graph reports storing fewer bytes than the 7 we sent → truncation. + json: async () => ({ eTag: 'etag-1', size: 3 }), + text: async () => '', + } as Response; + } + return { ok: true, status: 200, text: async () => '' } as Response; + }); + + try { + await provider.uploadFile('file-1.json', '{"a":1}', null, true); + fail('should have thrown'); + } catch (e) { + expect((e as Error).name).toBe('UploadRevToMatchMismatchAPIError'); + } + }); + it('should refresh token and retry on 401', async () => { let firstRequest = true; cfgStoreSpy.load.and.resolveTo(baseCfg);