Skip to content

Commit f143dfd

Browse files
committed
fix(sync): re-upload an attachment whose Dropbox or self-hosted blob is gone from the device that still holds it, and answer HEAD on cloud attachments (#1119)
1 parent 236f969 commit f143dfd

18 files changed

Lines changed: 1249 additions & 39 deletions

apps/cloud/src/server-attachments.test.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -578,3 +578,76 @@ describe('handleAttachmentPathRequest DELETE', () => {
578578
}
579579
});
580580
});
581+
582+
// #1119 attachment presence pass: a client that cannot stop a response body early must be
583+
// able to ask whether a blob is still there without downloading it.
584+
describe('handleAttachmentPathRequest HEAD', () => {
585+
const withSandbox = async (
586+
run: (paths: { rootRealPath: string; filePath: string }) => Promise<void>,
587+
): Promise<void> => {
588+
const sandbox = mkdtempSync(join(tmpdir(), 'mindwtr-cloud-attachment-head-'));
589+
try {
590+
const rootRealPath = join(sandbox, 'attachments');
591+
mkdirSync(rootRealPath, { recursive: true });
592+
await run({ rootRealPath, filePath: join(rootRealPath, 'file.bin') });
593+
} finally {
594+
rmSync(sandbox, { recursive: true, force: true });
595+
}
596+
};
597+
598+
const head = (paths: { rootRealPath: string; filePath: string }) => handleAttachmentPathRequest(
599+
new Request('http://localhost/v1/attachments/file.bin', { method: 'HEAD' }),
600+
'/v1/attachments/file.bin',
601+
paths,
602+
{ maxAttachmentBytes: 1024, abortSignal: new AbortController().signal },
603+
);
604+
605+
test('reports a stored attachment with its size and no body', async () => {
606+
await withSandbox(async (paths) => {
607+
writeFileSync(paths.filePath, 'attachment');
608+
609+
const response = await head(paths);
610+
611+
expect(response.status).toBe(200);
612+
expect(response.headers.get('content-length')).toBe(String('attachment'.length));
613+
expect(response.headers.get('content-type')).toBe('application/octet-stream');
614+
expect(await response.arrayBuffer()).toHaveLength(0);
615+
});
616+
});
617+
618+
test('answers 404 for an attachment that is not there', async () => {
619+
await withSandbox(async (paths) => {
620+
const response = await head(paths);
621+
expect(response.status).toBe(404);
622+
});
623+
});
624+
625+
test('agrees with GET about status and size', async () => {
626+
await withSandbox(async (paths) => {
627+
writeFileSync(paths.filePath, 'attachment');
628+
const getResponse = await handleAttachmentPathRequest(
629+
new Request('http://localhost/v1/attachments/file.bin'),
630+
'/v1/attachments/file.bin',
631+
paths,
632+
{ maxAttachmentBytes: 1024, abortSignal: new AbortController().signal },
633+
);
634+
const headResponse = await head(paths);
635+
636+
expect(headResponse.status).toBe(getResponse.status);
637+
expect(headResponse.headers.get('content-length'))
638+
.toBe(String((await getResponse.arrayBuffer()).byteLength));
639+
});
640+
});
641+
642+
test('never leaves the attachment root, even through a symlink', async () => {
643+
await withSandbox(async (paths) => {
644+
const outside = join(paths.rootRealPath, '..', 'outside.bin');
645+
writeFileSync(outside, 'secret');
646+
symlinkSync(outside, paths.filePath);
647+
648+
const response = await head(paths);
649+
650+
expect(response.status).toBe(400);
651+
});
652+
});
653+
});

apps/cloud/src/server-attachments.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,10 +268,15 @@ const getBlockedAttachmentSignature = (bytes: Uint8Array): string | null => {
268268
};
269269

270270
/**
271-
* Route body for GET/PUT/DELETE /v1/attachments/:path, once withNamespace has already
271+
* Route body for HEAD/GET/PUT/DELETE /v1/attachments/:path, once withNamespace has already
272272
* resolved and validated the on-disk path (see resolveAttachmentPath in
273273
* server-storage.ts). Takes the resolved path rather than resolving it itself, so it
274274
* can be exercised directly against a temp directory without a live server.
275+
*
276+
* HEAD shares GET's body below and drops the bytes at the end. It exists for the #1119
277+
* attachment presence pass: a client that cannot stop a response body early (React Native's
278+
* XHR transport buffers the whole reply before resolving) would otherwise have to download
279+
* every attachment to learn whether it is still there.
275280
*/
276281
export async function handleAttachmentPathRequest(
277282
req: Request,
@@ -286,7 +291,7 @@ export async function handleAttachmentPathRequest(
286291
): Promise<Response> {
287292
const { rootRealPath, filePath } = resolved;
288293

289-
if (req.method === 'GET') {
294+
if (req.method === 'GET' || req.method === 'HEAD') {
290295
options.assertStorageRoot?.();
291296
if (!existsSync(filePath)) {
292297
options.assertStorageRoot?.();
@@ -303,6 +308,12 @@ export async function handleAttachmentPathRequest(
303308
const headers = new Headers();
304309
headers.set('Access-Control-Allow-Origin', corsOrigin);
305310
headers.set('Content-Type', 'application/octet-stream');
311+
// ponytail: HEAD still reads the file, so its status and Content-Length cannot
312+
// disagree with GET's. Stat instead of read if presence checks ever get hot.
313+
if (req.method === 'HEAD') {
314+
headers.set('Content-Length', String(file.byteLength));
315+
return new Response(null, { status: 200, headers });
316+
}
306317
return new Response(file, { status: 200, headers });
307318
} catch {
308319
options.assertStorageRoot?.();

apps/cloud/src/server.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3758,6 +3758,20 @@ describe('cloud server api', () => {
37583758
const downloaded = new Uint8Array(await getResponse.arrayBuffer());
37593759
expect(Array.from(downloaded)).toEqual(Array.from(payload));
37603760

3761+
// #1119 presence pass: HEAD answers the same question without the bytes.
3762+
const headResponse = await fetch(`${baseUrl}/v1/attachments/folder/file.bin`, {
3763+
method: 'HEAD',
3764+
headers: authHeaders,
3765+
});
3766+
expect(headResponse.status).toBe(200);
3767+
expect(headResponse.headers.get('content-length')).toBe(String(payload.byteLength));
3768+
expect(new Uint8Array(await headResponse.arrayBuffer())).toHaveLength(0);
3769+
3770+
const unauthorizedHead = await fetch(`${baseUrl}/v1/attachments/folder/file.bin`, {
3771+
method: 'HEAD',
3772+
});
3773+
expect(unauthorizedHead.status).toBe(401);
3774+
37613775
const deleteResponse = await fetch(`${baseUrl}/v1/attachments/folder/file.bin`, {
37623776
method: 'DELETE',
37633777
headers: authHeaders,
@@ -3768,6 +3782,12 @@ describe('cloud server api', () => {
37683782
headers: authHeaders,
37693783
});
37703784
expect(missingResponse.status).toBe(404);
3785+
3786+
const missingHead = await fetch(`${baseUrl}/v1/attachments/folder/file.bin`, {
3787+
method: 'HEAD',
3788+
headers: authHeaders,
3789+
});
3790+
expect(missingHead.status).toBe(404);
37713791
});
37723792

37733793
test('fails a partial attachment upload when the configured storage root is replaced', async () => {

apps/desktop/src/lib/dropbox-sync.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ export {
99
getDropboxAppDataMetadata,
1010
getDropboxFileMetadata,
1111
isDropboxUnauthorizedError,
12+
listDropboxFolderFiles,
1213
testDropboxAccess,
1314
uploadDropboxAppData,
1415
uploadDropboxFile,

0 commit comments

Comments
 (0)