Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove inline javascript to comply with some CSP #34

Closed
mossroy opened this issue Dec 29, 2021 · 15 comments
Closed

Remove inline javascript to comply with some CSP #34

mossroy opened this issue Dec 29, 2021 · 15 comments

Comments

@mossroy
Copy link

mossroy commented Dec 29, 2021

For security reasons, some environments block inline javascript. It's the case with Kiwix-JS, at least when run inside a browser extension.

https://mirror.download.kiwix.org/zim/other/zimgit-post-disaster_en_2021-12.zim contains some inline javascript, at least in home.html (line 82), which prevents it to work in kiwix-js (even in ServiceWorker mode). The following error message appears in the browser console, and a spinner keeps turning

Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self' 'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-PtO5L7mz44xgm8o7YDaYSxpBLHPVArwXn4bgl0rjzNw='), or a nonce ('nonce-...') is required to enable inline execution.

image

The right way to fix this is to remove any inline javascript, and use only javascript files.
In this example, I suppose that moving the corresponding javascript inside nautilus.js should be enough?

@rgaudin
Copy link
Member

rgaudin commented Dec 29, 2021

I suppose you are talking about this piece of script

    <script type="text/javascript">
        $( document ).ready(function() {
            $('[data-toggle="tooltip"]').tooltip();
            window.nautilus = new Nautilus({
                title: "Zimgit-Post Disaster Resource Library",
                description: "This and more prepper material at zimgit.com",
                database_name: "zimgit-post-disaster_en_2021-12_ab1fa6347aa9474bab48cd22dd0feb54_db.1",
                database_version: "1",
                nb_items_per_page: 110,
                show_author: true,
                show_description: true,
                randomize: false,
                debug: true,
                i18n: {loading: "Loading…",},
            });
            window.nautilus.start();
        });
    </script>

nautilus.js is a static file at the moment ; might be easier/more-maintainable to generate a small JS file containing this code instead of making it a template altogether.

Will try to generate a new file for you to test.

@mossroy
Copy link
Author

mossroy commented Dec 29, 2021

Thanks @rgaudin

Yes, it's this piece of code that generates the error.
No problem for another javascript file. The need is to get rid of inline javascript.

@Popolechien
Copy link
Contributor

Popolechien commented Jan 5, 2022

Just tested with https://mirror.download.kiwix.org/zim/other/zimgit-post-disaster_en_2022-01.zim that was generated after the last release and the issue remains.
Don't know how related that is but here's what the dev console tells me:
image

@Popolechien Popolechien reopened this Jan 5, 2022
@mossroy
Copy link
Author

mossroy commented Jan 5, 2022

@Popolechien : to me, this issue has been fixed.
The ZIM file you mention appears to work fine in Chromium extension : no inline javascript is blocked.
The console warnings you mention are not related to this : they are minor warnings about developer tools not finding some extra info that help debugging

@Jaifroid
Copy link

I'm still getting a load failure in Chromium extension (more specifically, the Edge extension, which is exactly the same package as the Chrome extension) with https://download.kiwix.org/zim/other/zimgit-post-disaster_en_2022-03.zim . This one should be later than the 22-01 version above that was said to be fixed.

image

It loads fine in https.//moz-extension.kiwix.org (in Edge/Chrome), so this is a Chromium-extension issue still. Could there be a regression?

@rgaudin
Copy link
Member

rgaudin commented Sep 21, 2022

What would be the inline script ?

@Jaifroid
Copy link

Jaifroid commented Sep 21, 2022

Sorry, I realize I was only describing the symptoms, whereas I should investigate, but won't have time to do so till the w/e. I wanted to put down a marker here. I don't see any blocked inline JS in console, so it is something more complex and not self-evident. It may be an unrelated issue. Please keep issue closed until I can confirm whether what I'm seeing is related to this issue.

@Jaifroid
Copy link

OK, this isn't caused by inline JavaScript, but by a race condition (I'm guessing) with the loading of the favicon, which is used too early by a test in zimwriterfs. In Chromium browsers running from localhost or from a file extension, it appears the favicon is not loaded by the time the test is run. See screenshot.

image

This in turn causes nautilus.js to error out:

image

However, if I wait till the document is loaded fully before running the favicon test, I get a correct result in console.log:

document.getElementById("favicon").getAttribute("href").indexOf("/I/") !== -1;
true

I realize this is a separate issue, so I'll copy this comment into a new issue.

@rgaudin
Copy link
Member

rgaudin commented Sep 30, 2022

There shouldn't be any more inline JS now. Please test https://mirror.download.kiwix.org/zim/other/bayardcuisine_fr_2022-09.zim (updated since this morning) and confirm @Jaifroid

@Jaifroid
Copy link

I confirm that inline JS has now gone from the file, thank you @rgaudin for the swift resolution!

@mossroy We do have an issue finding the video files to play within the video.js widget in the Bayard Cuisine ZIM that @rgaudin has provided, but that problem has nothing to do with the CSP, as it occurs also in https://moz-extension.kiwix.org/ , which is not gated by the Extension CSP. There are no CSP violations in the console in the extension version.

It's an issue to investigate on our side... The user can click on the link to the video and it will open and play, so it's not a huge problem, but it would be good to understand what is going wrong in KJS code here. The video widgets work fine in Kiwix Desktop and in KJSW, so it's something specific going on in KJS, I think.

@Jaifroid
Copy link

Jaifroid commented Oct 1, 2022

Apologies, @rgaudin , I spoke too soon about this fix working in KJSW. In fact KJSW was accessing a cached version of the scripts. It has the same issue as KJS (the browser extension), which is that it is unable to locate the new file load_videojs.js. The Service Worker does not trap this load request, and so the file is not found (screenshot). The code you changed is here.

The problem is to do with the dynamic (re-)loading of load_videojs.js every time a user selects a video. Maybe because it's loaded by template-interpreting code, it is not caught by the Service Worker: it seems to be loaded outside of the Service Worker's scope. Every other request is caught, but not this one.

Instead of loading a file just for this one jQuery function, try including the code $('.video-js').each(function(){ videojs($(this)[0], videojs_options)}); just after the loading of the media player in nautlius.js (line 222 - I'll add a comment where this can be done).

I've tested this in the console by pausing on line 223 and running the command $('.video-js').each(function(){ videojs($(this)[0], videojs_options)}); in console. This works perfectly. The video.js block loads the video as expected. If you do this, then you can remove load_videojs.js as it won't be needed (it just contains that jQuery command).

image

@Jaifroid
Copy link

Jaifroid commented Oct 1, 2022

It won't let me add a comment to the PR on line 222, but it's here:

https://github.com/openzim/nautilus/blob/master/nautiluszim/templates/nautilus.js#L222

Just add $('.video-js').each(function(){ videojs($(this)[0], videojs_options)}); straight after line 222, inside the same closure.

@rgaudin
Copy link
Member

rgaudin commented Oct 1, 2022

@Jaifroid I've made the fix but I don't think it's a reasonable request. Not being able to handle a new DOM element that sources an external resource seems like a reader issue… something even zimit handles well within its Service Worker. I'd advise you look it up.
Again, confirm the updated ZIM is OK. I've tested with kiwix-js and I can play videos just fine.

@Jaifroid
Copy link

Jaifroid commented Oct 1, 2022

@rgaudin I don't understand why the Service Worker was not trapping that newly attached DOM element, but I can say for sure it was not, and that would be a road block for us, very hard to work around. I can only think that it has something to do with the way the templating system was attaching the element internally (maybe via some eval?), and whether it was truly in scope of the Service Worker when first attached (nothing to do with your code, maybe to do with the template mechanism).

Normally adding a new DOM element is not a problem for us -- otherwise lots of things would be completely broken, I agree!

Many thanks indeed for putting the suggested code in and for testing it on the browser extension. When it refreshes on the mirror, I'll test.

@Jaifroid
Copy link

Jaifroid commented Oct 1, 2022

Yes, I confirm it works! Thank you so much - it makes a big difference to Kiwix JS. I know we're always the "odd one out", because doing things in JavaScript means we're always grappling with oddities, browser bugs, and the limits of what the technology can do, but I hope it is still worth it for Kiwix to have the kind of universal accessibility that a browser-based reader can offer to users. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants