Added a feature that prompts a user when a new Zim file is opened.

[Greeshmanth1909]

Might resolve #974
An alert will appear when a user opens a file for the first time asking weather they trust the source of the file or not. If they do, the file will be read in the usual way. If the user does not trust the source, the file will be read in JQuery mode.
The option to disable this feature was not added.
Once the user says they trust a particular file, the same prompt will not appear for that file in the future.

[Greeshmanth1909]

@Jaifroid I was able to view and browse files once I confirmed the source with the 'trust' button in the pop up.
The alert box that warns the user before opening a new file might be causing the errors we are seeing here imo. What do you think?

[Jaifroid]

I'm pretty sure the tests are failing due to the alert. I think you'll have to add an option to bypass this (trust all local ZIM archives), and enable the bypass for tests. You can add a simple params.trustAllLocalZIMArchives Boolean for now, and set it to true for tests.

[Greeshmanth1909]

@Jaifroid Only one of the e2e tests failed. Can you re-run?

[Jaifroid]

A couple of quick observations before you go much further with this PR:

• You seem to be asking the user if they trust the ZIM in the readArticle function. However, that seems too late to me, especially as the query to the user is async, so in fact the readArticle will go ahead while the user's consent is being requested (I haven't checked your code in detail, so not sure if you accounted for this already). However, it seems to me that this question should be asked in archiveReadyCallback, i.e. once the archive is ready, but before goToMainAritcle is called, and hence before any assets (JS in particular) can be loaded.
• I think it will be necessary to allow the user to turn off the warnings completely in the UI (probably an Expert Setting, with appropriate warning about security threat).

[Greeshmanth1909]

The user can enable this feature in the expert settings via the "Enable source verification" checkbox which will be unchecked by default for testing purposes. This can however be changed to have the feature checked by default if necessary.

[Jaifroid]

The user can enable this feature in the expert settings via the "Enable source verification" checkbox which will be unchecked by default for testing purposes. This can however be changed to have the feature checked by default if necessary.

I would say that the feature should be on by default, since it is a security feature: users should be protected by default. In a testing context, of course, it will be turned off in code.

[Jaifroid]

Also, you will see that code scanning is producing a high-risk alert. I'm not at all sure why it's being flagged as high risk -- I can only assume it is based on the naming of the variables (with the word trusted) that is flagging this as not a trustworthy way of storing trusted data.

What is your assessment of the actual risk here? I can't see a way of encrypting the data without including the keys for encrypting and decrypting them in the app. Since this is an offline solution, we can't use server-based encryption/decryption.

[Greeshmanth1909]

This feature can be enabled by default by simply changing a Boolean to true. I disabled it by default so that it can be tested, thinking it can be changed in production.

Either we automatically disable this during testing or tweak the tests to accommodate this feature.

Do you see another way @Jaifroid ?

[Greeshmanth1909]

We store the names of the zim files that the user flags as "trusted" in local storage as a continuous string of file names separated by "|".
I named the key of this value as 'trustedZimFiles' for simplicity.
As we are only reading the file names, I don't see any security risk here.

[Greeshmanth1909]

Should I try changing the key to something else?

[Jaifroid]

On security, I agree. I think this is a false positive based on matching the word "trusted". However, I don't think we should change the variable name because of that.

On the default, I suggest turning the feature on by default. In the testing script(s) you then need to set the appropriate param so that tests will be able to load the ZIMs they test against without interruption. Note that Unit tests contain their own init.js, so you could set the param there. CI tests will also need adjusting in the scripts that run those, or by testing for a CI environment. Note that we test on GitHub, but we also test on Browser Stack, but the Browser Stack tests don't run for third-party contributors like yourself. I will need to add a mirror PR for them to run.

[Greeshmanth1909]

So I just have to manually change the params object in the tests/unit/js/init.js
I don't completely understand what to do for the CI test though.

[Jaifroid]

Yes, for Unit tests, just set the param in that init.js. For the end-to-end tests, there are test files legacy-ray_charles.e2e.spec.js and gutenberg_ro.e2e.spec.js under tests/e2e/spec/. These tests run against compiled versions of the code, so you can't just edit an init file. There are two approaches I can think of:

1. Write code in these test files that tests for and dismisses the alert box when the archives are opened (you can copy the style of code for dismissing other alerts already in these files - search for "dismiss");
2. Alternatively, set the params.sourceVerifiaction Boolean by passing it to the app by using await driver.executeScript(...). This would need to be done after the app has loaded, but before the archives are loaded into the app in these scripts.

The first would be the more useful in a way because it would also be testing your code's UI. The same test runners run in GitHub actions as in Browser Stack, so you should be able to see if your code is working in your PR, and when it's working with the GitHub e2e tests, I can make a mirror PR (which I won't merge) that will allow your code to run on Browser Stack as well, to be sure all is working well.

[Greeshmanth1909]

Thanks a lot for the info!
I'll fix this asap.

[Greeshmanth1909]

I've made the necessary changes, please go ahead with the Browser Stack tests @Jaifroid

[Jaifroid]

@Greeshmanth1909 Thanks! I ran the Linux tests twice. The first one appeared to be a random failure, but the second one looks like an issue with the PR. You can see the error messages here: https://github.com/kiwix/kiwix-js/actions/runs/7708040000/job/21006685376?pr=1192, and you can examine the session on Browser Stack (and view the video) here: https://automate.browserstack.com/builds/02e2058e5efa9ad2a556c82e00ff2ebaa6322d33/sessions/39e1472142df23ca14a8c9ea71fc746bd2081e29?auth_token=e6d8704fd572b3fdee6dfa06f7d84dbc7367cdf8f437f38e5db0ed06439297d5

One thing I noticed is that the failed session was already running in jQuery mode, but your dialogue box asking if the user trusts the source still popped up. If you add code to prevent the popup showing if the user is already in jQuery mode (just check params.contentInjectionMode) it will allow us to progress these tests to the next stage and see if they are also failing in SW mode. (And in any case, I don't think the popup should show in jQuery mode, as it's redundant.)

The tests will need to be adjusted also so that the detection of the dialogue box doesn't occur in the jquery test. You can easily tell in the test code if it is running in jquery or sw mode with mode === 'jquery' (will be true or false).

[Jaifroid]

NB I think the cause of the failure is probably a timeout due to the fact that that Firefox test is running on a very old version (56), albeit one we actively support. However, that Firefox only supports jQuery mode, so we shouldn't adjust timings just yet. If you eliminate the dialogue box in jQuery mode, then a number of the tests on very old browsers should run as normal, and then we can focus on the newer browsers.

[Greeshmanth1909]

I've translated the strings into French and Spanish using deepl.

[Jaifroid]

Thanks! Sorry for slow response -- was busier than I thought at w/e.

[Jaifroid]

Many thanks for making a stab at the translations! I've added my suggested changes, and there are a few strings you forgot. In this review, I only focused on the translation strings, and didn't test the code. I'll do that for the final time when you've completed the UI.

UI is always pesky: lots of fiddly details that coders don't really see the point of, but users always obsess over... So, it's like the icing on the cake that makes the whole thing desirable form a UX perspective!

[Greeshmanth1909]

I should get this done by tonight!

[Greeshmanth1909]

There's a 110% chance I missed something here, lol.

[Jaifroid]

Last few now! Hopefully we'll be done pending final testing.

[Greeshmanth1909]

The same test failed last time too, is this something that we should be worried about?

[Jaifroid]

The same test failed last time too, is this something that we should be worried about?

This is just a flaky test, unfortunately. There's some race condition because sometimes it works and sometimes it fails, and it seems to depend on the overall load on the system...

[Jaifroid]

Congratulations on this PR! It all looks good to me now, apart from one tiny little correction below. I'll do a bit more testing of the switch from a local extension to the remote code and vice versa, but in the meantime, after you've done the one little correction, let me know if you're happy for the PR to be merged.

[Greeshmanth1909]

Thanks! Please go ahead and merge this PR.

[Jaifroid]

Just to confirm that the flow seems to work well from JQuery mode in an extension through to SW mode. The user will likely be asked twice if they trust the source because the list of secure archives in the local code is not the same list as is maintained in remote code -- but this is inevitable, and will only affect a user who is switching to SW mode for the first time and has an archive loaded (a rare situation, since most users capable of running the app in SW mode will have switched when they first loaded the app). So, all OK! Thanks once again for this PR which will increase people's awareness of security. I'll merge when the Linux test is complete.