Clipper: Add capability of limiting downloads #9788

pedr · 2024-01-26T19:14:17Z

Summary

Creating a new abstraction DownloadLimiter that can be used to limit the amount of data that extractNoteFromHTML can download. This works by counting how many images and the number of bytes downloaded by the shim.fetchBlob calls.

Changes

Limit how much data can be downloaded by the extractNoteFromHTML.
~~Change the "promise-pool" library.~~
Add a log when the limit is reached
Add a header to a note body if the limit is reached.

Why change the promise-pool library

Initially I planned to change the library that we were using to handle the image downloads, after some discussing we agreed that Promise.all should be enough for our use case.

Log pattern:

2024-01-20 00:29:43: downloadController: Owner id: qcc9jPy9UZ5xSuBhdBkSTZ - Total bytes stored: 841811. Maximum: 52428800 - Images initiated for download: 12. Maximum: 11. Expected: 57

2024-01-20 00:29:43: downloadController: Owner id: {ownerId} - Total bytes stored: {how much was downloaded in the request}. Maximum: { value set in the LimitedDownloadController } - Images initiated for download: { how many images started to be downloaded}. Maximum: { value set in the LimitedDownloadController }. Expected: { how many image URLs we had in the email body}

Implementation

There are "two behaviors".

When the image count limit is reached.
We can stop new downloads from even starting, there is only one warn log message since we can control it:

When the total number of bytes is exceeded.
Since the only place we can check this is inside the fetchBlob function and the error is being controlled by a try-catch there we can are potential logging a lot of error messages:

Testing

This change should not affect anything since the current code only uses a DummyDownloadController class as the fallback.

Any unexpected behavior from the Joplin Web Clipper or any code that uses shim.fetchBlob is a potential bug.

laurent22 · 2024-01-26T19:20:19Z

packages/lib/downloadController.ts

+	private bytesToHuman(bytes: number) {
+		const units = ['Bytes', 'KB', 'MB', 'GB'];
+		let unitIndex = 0;
+
+		while (bytes >= 1024 && unitIndex < units.length - 1) {
+			bytes /= 1024;
+			unitIndex++;
+		}
+
+		return `${bytes.toFixed(1)} ${units[unitIndex]}`;
+	}


Let's move this to @joplin/utils/bytes

laurent22 · 2024-01-26T19:23:03Z

packages/lib/downloadController.ts

+	public limitMessage() {
+		if (this.imagesCount_ > this.maxImagesCount) {
+			return `Your email has ${this.imageCountExpected} images, exceeding the limit of ${this.maxImagesCount}.`;
+		}
+		if (this.totalBytes >= this.maxTotalBytes) {
+			return `The size of your email (${this.bytesToHuman(this.totalBytes)}) was larger than the allowed limit (${this.bytesToHuman(this.maxTotalBytes)}).`;
+		}
+		return '';
+	}


This is a generic controller that could apply to anything, not just emails, so we should remove references to emails in the error messages.

laurent22 · 2024-01-26T19:25:08Z

packages/lib/services/rest/routes/notes.ts

@@ -223,7 +226,7 @@ const isValidUrl = (url: string, isDataUrl: boolean, urlProtocol?: string, allow
 	return isAllowedProtocol;
 };

-export async function downloadMediaFile(url: string, fetchOptions?: FetchOptions, allowedProtocols?: string[]) {
+export async function downloadMediaFile(url: string, downloadController: DownloadController, fetchOptions?: FetchOptions, allowedProtocols?: string[]) {


Please make the downloadController optional.

I know we have this rule to avoid optional parameters, but when it means having to create a dummy object on each call it's better to make it optional

Also why do we need this dummy controller? Isn't it possible to make the code work with a simple null controller? I guess there will be a few null checks here and there.

Especially since fetchBlob is heavily used for sync it's better to avoid creating unnecessary objects on each call.

And if it's optional, make it part of the "fetchOptions" object

I move the downloadController to be a property of the other options object and instead of creating a dummy class I'm checking before using downloadController like you suggested.

laurent22 · 2024-01-26T19:28:47Z

packages/lib/shim-init-node.ts

@@ -471,9 +472,10 @@ function shimInit(options: ShimInitOptions = null) {
 		}, options);
 	};

-	shim.fetchBlob = async function(url: any, options) {
+	shim.fetchBlob = async function(url: any, options: any, downloadController?: DownloadController) {


The options parameter by convention should be the last one, so it means adding downloadController as an option

packages/lib/downloadController.ts

laurent22 · 2024-01-26T19:31:32Z

packages/lib/downloadController.ts

+		return this.imageCountExpected_;
+	}
+
+	public handleNewChunk(request: any) {


Small nitpick but "handleNewChunk" is not necessary it's just handleChunk

pedr · 2024-02-22T13:59:05Z

This is a case where using the promising-pool lib is better than Promise.all. If we were using Promise.all we wouldn't be able to stop the execution midway (for example, if the content that is being downloaded exceeded a size limit that we choose).

pedr · 2024-02-28T13:13:03Z

We talked about having each operation throw an error when the limit is reached instead of the class having a state that identifies if the limit has been reached. The problem with this approach is that if we have 100 image links inside the content and the size limit is reached at the 10th image we will be logging 90 error messages.

I'm not even sure how I could avoid this kind of error logging flood because it is a behavior that happens inside fetchBlob function, so the only way I can think would be to use a try catch to suppress the errors that are related to this feature (I have an error code to identify it).

laurent22 · 2024-03-02T14:34:38Z

packages/lib/services/rest/routes/notes.ts

-	let urlIndex = 0;
-	const promiseProducer = () => {
-		if (urlIndex >= urls.length) return null;
+	const maximumImageDownloadsAllowed = downloadController ? downloadController.maxImagesCount : urls.length;


How about setting it to Number.POSITIVE_INFINITY if downloadController is not defined? It seems more future proof since it will work even if somehow the number of urls change in the future (if something's appended to the array)

packages/lib/services/rest/routes/notes.ts

laurent22 · 2024-03-02T14:37:09Z

packages/lib/shim-init-node.ts

 		if (!options || !options.path) throw new Error('fetchBlob: target file path is missing');
 		if (!options.method) options.method = 'GET';
+		if (!options.downloadController) options.downloadController = null;


That's not necessary

laurent22 · 2024-03-02T14:39:42Z

packages/lib/shim-init-node.ts

@@ -571,6 +583,11 @@ function shimInit(options: ShimInitOptions = null) {
 					});

 					const request = http.request(requestOptions, (response: any) => {
+
+						if (downloadController !== null) {


No need for strict null checking here. What you want to check is if it's defined or not and for that just if (downloadController) is enough. There are places maybe where we need to make the difference between undefined and null or other falsy values, but not when we just need to know if something exists or not.

laurent22 · 2024-03-02T14:42:41Z

packages/lib/utils/bytes.ts

+// eslint-disable-next-line import/prefer-default-export
+export const bytesToHuman = (bytes: number) => {
+	const units = ['Bytes', 'KB', 'MB', 'GB'];
+	let unitIndex = 0;
+
+	while (bytes >= 1024 && unitIndex < units.length - 1) {
+		bytes /= 1024;
+		unitIndex++;
+	}
+
+	return `${bytes.toFixed(1)} ${units[unitIndex]}`;
+};


Hmm, but I've suggested moving it to @joplin/utils so why is it in @joplin/lib now? /lib should be mostly for business logic, specific to Joplin. /utils is for generic utilities with no dependencies that can be used anywhere. This is so other packages don't need to import the whole lib, renderer, editor and more when all they need is some simple utilities.

Ah ok, my bad, I'm fixing this.

pedr added 3 commits January 26, 2024 15:52

add downloadController classes

e5b4811

implementing downloadLimiter to extractNote

11d2be7

add tests

67f5020

pedr requested a review from laurent22 January 26, 2024 19:14

pedr changed the title ~~Clipper: Add capability of setting download limit~~ Clipper: Add capability of limiting downloads Jan 26, 2024

commiting yarn lock

d602b72

laurent22 reviewed Jan 26, 2024

View reviewed changes

laurent22 added the v2.15 label Feb 6, 2024

pedr added 5 commits February 27, 2024 19:20

moving bytesToHuman to its a bytes module

305ae73

improving grammar

7cf098a

moving downloadController to fetchBlob options

e3fb948

renaming

816fafb

removing dummy and checking before using download controller

4f9b646

pedr added 4 commits February 28, 2024 10:25

Removing third party dependency

0398f56

fixing tests

6823ab5

Merge remote-tracking branch 'upstream/dev' into download-controller

4bbb561

update yarn lock

a7c89b5

laurent22 reviewed Mar 2, 2024

View reviewed changes

pedr added 4 commits March 4, 2024 17:48

fix fetchOptions being a optional argument

1aa1c9b

replacing urls length with infinity

f72323d

removing unnecessary code

cfcad1e

moving utility function to correct package

f568ede

laurent22 merged commit 17a8ce5 into laurent22:dev Mar 9, 2024
10 checks passed

pedr deleted the download-controller branch March 10, 2024 23:51

datatheorem-phillip mentioned this pull request May 22, 2024

[Snyk] Fix for 2 vulnerabilities datatheorem-phillip/joplin#207

Open

darcy168 mentioned this pull request May 22, 2024

[Snyk] Fix for 2 vulnerabilities darcy168/joplin#201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clipper: Add capability of limiting downloads #9788

Clipper: Add capability of limiting downloads #9788

pedr commented Jan 26, 2024 •

edited

Loading

laurent22 Jan 26, 2024

laurent22 Jan 26, 2024

laurent22 Jan 26, 2024

laurent22 Jan 26, 2024

laurent22 Jan 26, 2024

pedr Feb 27, 2024

laurent22 Jan 26, 2024

laurent22 Jan 26, 2024

pedr commented Feb 22, 2024 •

edited

Loading

pedr commented Feb 28, 2024

laurent22 Mar 2, 2024

laurent22 Mar 2, 2024

laurent22 Mar 2, 2024

laurent22 Mar 2, 2024

pedr Mar 4, 2024

Clipper: Add capability of limiting downloads #9788

Clipper: Add capability of limiting downloads #9788

Conversation

pedr commented Jan 26, 2024 • edited Loading

Summary

Changes

Why change the promise-pool library

Log pattern:

Implementation

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedr commented Feb 22, 2024 • edited Loading

pedr commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedr commented Jan 26, 2024 •

edited

Loading

pedr commented Feb 22, 2024 •

edited

Loading