Clipper: Increase protection of files downloaded by webclipper and cloudRestrict download media file

[pedr]

I'm changing the current behavior from downloadMediaFile from a invalidProtocols list to an allowedProtocols list, which have http, https, data and file as the default values.

I also added more automated tests to the downloadMediaFile function and refactored the code to be more readable, basically grouping things together in functions.

I think the best way to read this PR is by looking at each individual commit.

[laurent22]

Thanks Pedro, that looks good. I've just left a few comments regarding the test units.

Also please could you fix the pull request title?

[laurent22]

protocls => protocols

[laurent22]

I would prefer if we avoid using spy as much as possible because it tests implementation details, and the more we do this the harder it becomes to refactor the code. For example if we start using fs-extra instead of fsDriver this test would be broken but there's no reason for that to happen since the input/output of the function hasn't changed.

In that particular case, isn't it possible to verify that the function simply did nothing (didn't download any media file)?

[laurent22]

I guess it makes sense to mock fetchBlob to avoid http requests in tests, but for the other mocks I'm not sure, I think they can be avoided. It's nice to know in advance the UUID but again if we use another uuid lib, this test will be broken. Maybe you can check what's in the directory before and after to find what you need.

[pedr]

This is trickier to do without spyOn.

What I'm testing is when the image type can't be found by the URL.

We fetch the image and save it on filesystem with fetchBlob function, since we don't want a HTTP request in the test I'm using a mock for it, so no file gets created. Then the code will call tryToGuessExtFromMimeType, since the URLs that I used for the test don't have an extension, which will try to move the file that was never created (that is why I have a mock for shim.fsDriver.move)

In the end we can't even look at the temp directory to see if the test worked because no files has ever been created in this test. Maybe it would be best to remove this test altogether?

[laurent22]

@pedr, another issues with spy is that it seems they are not cleared properly after the test, even though jest.resetAllMocks() is in beforeEach. After adding database setup to the tests, they started failing I think due to uuid.create not being restored properly (some object that should have an ID didn't have one anymore). I could only get them to work again using mockRestore, like so:

const fetchBlobSpy = jest.fn();
const spy1 = jest.spyOn(shim, 'fetchBlob').mockImplementation(fetchBlobSpy);
const spy2 = jest.spyOn(uuid, 'create').mockReturnValue('mocked_uuid_value');

const response = await downloadMediaFile(url);

expect(response.endsWith('mocked_uuid_value.png')).toBe(true);
expect(fetchBlobSpy).toBeCalledTimes(1);

spy1.mockRestore();
spy2.mockRestore();

I don't understand why because you've added jest.resetAllMocks() but anyway to be safe let's add mockRestore within the test itself. Or better, let's avoid spy methods when possible

[pedr]

I don't understand why because you've added jest.resetAllMocks() but anyway to be safe let's add mockRestore within the test itself. Or better, let's avoid spy methods when possible

I didn't notice this problem. This was probably one of the reasons that I choose to use mock so much, since I couldn't use the fsDriver() function (I had mocked before, but it wasn't being reset).

I rewrote some tests to remove some mocks and use the Setting.value('tempDir') to check for the file creation.

[laurent22]

I don't think it's necessary to mock uuid.create - you can simply check that "some file" exists. You can also check the file content to verify it's the right file. fetchBlob makes more sense

[laurent22]

The spy shouldn't be needed since you're mocking fetchBlob and can make it create real files?

[pedr]

Hi, I think now it is closer to your vision. At first, I thought that creating a file inside fetchBlob would be weird for the test, but since it is pretty simple and makes the test more meaningful I think it is the correct choice.

Ready for review again.