kiwix / kiwix-js

Fully portable & lightweight ZIM reader in Javascript
https://www.kiwix.org/
GNU General Public License v3.0
311 stars 135 forks source link

Added ZIM metadata to security dialogue box when opening ZIM for the first time #1250

Closed D3V-D closed 6 months ago

D3V-D commented 6 months ago

Fix for #1247 Ran npm run serve and npm run test and npm run preview Also did npm run build followed by npm run serve and tested on the /dist page.

Tested firefox + chromium. (not tested on ie11 yet)

Pretty sure jQuery mode testing is not applicable as the security dialogue is only for serviceWorker mode (correct me if I'm wrong).

Added french and spanish translations (and English) with deepl, feel free to double check.

Please review, @Jaifroid . Thanks!

D3V-D commented 6 months ago

Just wanted to add, there's a lot of other ZIM metadata I think could be used well here; so I don't know if we'd want to pursue that? Like including date of creation, description, etc.

Jaifroid commented 6 months ago

Just wanted to add, there's a lot of other ZIM metadata I think could be used well here; so I don't know if we'd want to pursue that? Like including date of creation, description, etc.

There are indeed, but each one has a "cost", in that we have to load them up-front from the ZIM for them to be available at the time these metadata are shown. Currently, most metadata aren't needed at ZIM load time, and they load in the background in order to prioritize opening the ZIM as fast as possible for the user. So I judged that these are the most important ones (when I coded it for the PWA).

Jaifroid commented 6 months ago

I've tested on Edge -- all functionality appears to work as intended. In IE11 of course the security dialogue doesn't show because it's already running in jQuery, aka "Safe", mode, but the code doesn't cause any issues for the browser. So, all working well!

I'll do a code review asap.

D3V-D commented 6 months ago

Yep, please let me know if I did something strange when retrieving the metadata for the alert, the solution felt somewhat obtuse.

D3V-D commented 6 months ago

Also, did you link this PR to the corresponding issue?

Jaifroid commented 6 months ago

Also, did you link this PR to the corresponding issue?

Done. Normally it links automatically if you write "Fixes .....", maybe because you wrote "Fix for ..." (and it's not a very intelligent algorithm)!

Jaifroid commented 6 months ago

Actually the algo is more intelligent than I give it credit for:

image

D3V-D commented 6 months ago

@Jaifroid many thanks for the feedback! Yeah I did think of passing in HTML directly, but I wasn't sure if that was a good idea or not, but I guess it would be better for the reasons you stated.

I'll also look into the different ways of getting the metadata, and get back to you with an update

Jaifroid commented 6 months ago

@Jaifroid many thanks for the feedback! Yeah I did think of passing in HTML directly, but I wasn't sure if that was a good idea or not, but I guess it would be better for the reasons you stated.

I'll also look into the different ways of getting the metadata, and get back to you with an update

Thanks! I don't think it will take very long, as it's mostly moving a few things around.

D3V-D commented 6 months ago

Alright, everything should be in order now. Please review again (and any testing necessary to ensure it's still functional, but it all seems fine on my end). @Jaifroid

Jaifroid commented 6 months ago

I also just noticed this, which doesn't show on main. You are probably using textContent somewhere and it might literally interpret   as text rather than as HTML? You can probably just substitute innerHTML at the appropriate point.

image

D3V-D commented 6 months ago

Yeah, I'll take a look at all those things.

D3V-D commented 6 months ago

Okay, I've fixed pretty much all the things you said, just one issue.

When putting the text for the verification metadata, I avoided using innerHTML and instead used innerText to avoid (since the metadata can be "spoofed") any kind of security issues. However, this makes it hard to use the   for French. If necessary to use that instead of a normal blank space, I can try using innerHTML for the translated text then followed by innerText to insert the actual metadata.

Jaifroid commented 6 months ago

OK, great. I understand the security concern with innerHTML if we are inserting a string that we do not control and may be a vector for attack. We do, of course, control the metadata labels, so it should be safe to insert at least those with innerHTML. The alternative would be to sanitize the metadata themselves, potentially in the getMetadata function (or whatever it's called).

Jaifroid commented 6 months ago

Actually, no need to complicate the code, as you're right that we don't actually need   where a space will do and there is no risk of the colons being orphaned (no screen will ever be that narrow). So leave it as you have it now.

D3V-D commented 6 months ago

All done! Feel free to merge whenever @Jaifroid

Jaifroid commented 6 months ago

Great, thanks for the PR!