From 54262ca3d6efd42e62e6f12d7a2ef32a5a61b8df Mon Sep 17 00:00:00 2001 From: Philipp Trulson Date: Thu, 10 Oct 2024 10:08:55 +0200 Subject: [PATCH 1/4] Move Content-Type check in front of Content-Length --- ts/linkPreviews/linkPreviewFetch.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ts/linkPreviews/linkPreviewFetch.ts b/ts/linkPreviews/linkPreviewFetch.ts index 97e0de7bce..bb9845e7f5 100644 --- a/ts/linkPreviews/linkPreviewFetch.ts +++ b/ts/linkPreviews/linkPreviewFetch.ts @@ -567,6 +567,14 @@ export async function fetchLinkPreviewImage( return null; } + const { type: contentType } = parseContentType( + response.headers.get('Content-Type') + ); + if (!contentType || !VALID_IMAGE_MIME_TYPES.has(contentType)) { + logger.warn('fetchLinkPreviewImage: Content-Type is not an image; bailing'); + return null; + } + const contentLength = parseContentLength( response.headers.get('Content-Length') ); @@ -581,14 +589,6 @@ export async function fetchLinkPreviewImage( return null; } - const { type: contentType } = parseContentType( - response.headers.get('Content-Type') - ); - if (!contentType || !VALID_IMAGE_MIME_TYPES.has(contentType)) { - logger.warn('fetchLinkPreviewImage: Content-Type is not an image; bailing'); - return null; - } - let data: Uint8Array; try { data = await response.buffer(); From 326d7b18941c48f285576695edbe71acd7cc41be Mon Sep 17 00:00:00 2001 From: Philipp Trulson Date: Thu, 10 Oct 2024 10:10:05 +0200 Subject: [PATCH 2/4] Assume max allowed size if Content-Length is not present --- ts/linkPreviews/linkPreviewFetch.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/ts/linkPreviews/linkPreviewFetch.ts b/ts/linkPreviews/linkPreviewFetch.ts index bb9845e7f5..41243169ba 100644 --- a/ts/linkPreviews/linkPreviewFetch.ts +++ b/ts/linkPreviews/linkPreviewFetch.ts @@ -177,6 +177,11 @@ const isInlineContentDisposition = (headerValue: string | null): boolean => !headerValue || headerValue.split(';', 1)[0] === 'inline'; const parseContentLength = (headerValue: string | null): number => { + if (headerValue == null) { + // If no Content-Length header is given, assume the max allowed value + return MAX_IMAGE_CONTENT_LENGTH; + } + // No need to parse gigantic Content-Lengths; only parse the first 10 digits. if (typeof headerValue !== 'string' || !/^\d{1,10}$/g.test(headerValue)) { return Infinity; @@ -583,9 +588,7 @@ export async function fetchLinkPreviewImage( return null; } if (contentLength > MAX_IMAGE_CONTENT_LENGTH) { - logger.warn( - 'fetchLinkPreviewImage: Content-Length is too large or is unset; bailing' - ); + logger.warn('fetchLinkPreviewImage: Content-Length is too large; bailing'); return null; } From 10601d4d88d5f306634e2a34c95596909c67d956 Mon Sep 17 00:00:00 2001 From: Philipp Trulson Date: Thu, 10 Oct 2024 10:10:24 +0200 Subject: [PATCH 3/4] Add second check for image size in case Content-Length is missing --- ts/linkPreviews/linkPreviewFetch.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ts/linkPreviews/linkPreviewFetch.ts b/ts/linkPreviews/linkPreviewFetch.ts index 41243169ba..ae46187424 100644 --- a/ts/linkPreviews/linkPreviewFetch.ts +++ b/ts/linkPreviews/linkPreviewFetch.ts @@ -600,6 +600,12 @@ export async function fetchLinkPreviewImage( return null; } + // Second check for image size in case there is no Content-Length header + if (data.length > MAX_IMAGE_CONTENT_LENGTH) { + logger.warn('fetchLinkPreviewImage: Image size is too large; bailing'); + return null; + } + if (abortSignal.aborted) { return null; } From 0e3d214d08515cd2b2907173706f30343859276e Mon Sep 17 00:00:00 2001 From: Philipp Trulson Date: Thu, 10 Oct 2024 10:38:21 +0200 Subject: [PATCH 4/4] Add test and adjust existing one for Content-Length --- .../linkPreviews/linkPreviewFetch_test.ts | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/ts/test-electron/linkPreviews/linkPreviewFetch_test.ts b/ts/test-electron/linkPreviews/linkPreviewFetch_test.ts index 462c1827c9..242f86cfa8 100644 --- a/ts/test-electron/linkPreviews/linkPreviewFetch_test.ts +++ b/ts/test-electron/linkPreviews/linkPreviewFetch_test.ts @@ -1280,7 +1280,7 @@ describe('link preview fetching', () => { ); }); - it('returns null if the response is too large', async () => { + it('returns null if the Content-Length is too large', async () => { const fakeFetch = stub().resolves( new Response(await readFixture('kitten-1-64-64.jpg'), { headers: { @@ -1302,7 +1302,32 @@ describe('link preview fetching', () => { sinon.assert.calledOnce(warn); sinon.assert.calledWith( warn, - 'fetchLinkPreviewImage: Content-Length is too large or is unset; bailing' + 'fetchLinkPreviewImage: Content-Length is too large; bailing' + ); + }); + + it('returns null if the actual image is too large', async () => { + const fakeFetch = stub().resolves( + new Response(await readFixture('tina-rolf-269345-unsplash.jpg'), { + headers: { + 'Content-Type': 'image/jpeg', + }, + }) + ); + + assert.isNull( + await fetchLinkPreviewImage( + fakeFetch, + 'https://example.com/img', + new AbortController().signal, + logger + ) + ); + + sinon.assert.calledOnce(warn); + sinon.assert.calledWith( + warn, + 'fetchLinkPreviewImage: Image size is too large; bailing' ); });