mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
* 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.
This commit is contained in:
parent
df491086c9
commit
ae4d9d7104
6 changed files with 213 additions and 1 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
57
packages/sync-providers/src/file-based/verify-upload-size.ts
Normal file
57
packages/sync-providers/src/file-based/verify-upload-size.ts
Normal file
|
|
@ -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.`,
|
||||
);
|
||||
}
|
||||
};
|
||||
|
|
@ -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<typeof vi.fn>).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({
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue