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.

Sign up for GitHub

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

Jump to bottom

Remove inline JavaScript to comply with some CSPs #145

Closed
Jaifroid opened this issue Jan 4, 2022 · 10 comments · Fixed by #189
Closed

Remove inline JavaScript to comply with some CSPs #145

Jaifroid opened this issue Jan 4, 2022 · 10 comments · Fixed by #189
Assignees
@benoit74
Labels
enhancement good first issue
Milestone
2.1.0

Comments

@Jaifroid
Copy link

Jaifroid commented Jan 4, 2022 •
edited

This is the same issue in Gutenberg that has been reported for PhET here: openzim/phet#134 . To summarize, Content Security Policies of browser extensions forbid running inline JavaScript. Our ZIMs should avoid having JS inline, and should put required blocks in separate js files. For example, in the landing page of Gutenberg ZIMs, we have:

<body onload="init(); showBooks(); " class="pure-skin-gutenberg home">

This could easily be replaced with a much safer bit of code in the JS file (tools.js) where init() and showBooks() are defined. All that would be needed is something like:

document.querySelector('body').addEventListener('load', function() {
    init();
    showBooks();
});

For more info, see kiwix/kiwix-js#789.

The effect of the current inline JS can be seen in our Chromium extension. Compare the left screenshot from the PWA version with the screenshot from the Chromium extension (both running in Service Worker mode, and both capable of running JS). The books are not shown in the extension because init() and showBooks() are blocked by the CSP.

image
@eshellman
Copy link
Collaborator

The base.html template fills the onload attribute this way:
onload="init(); {% if show_books %} showBooks(); {% else %} populateFilters(); {% endif %} {%if bookshelf_home %} showBookshelfSearchResults(''); {%endif%} {%if bookshelf is defined%} showBookshelf({{ bookshelf }} ); {%endif%}
so the show_books, bookshelf_home and bookshelf attributes would need to be exposed to the javascript somehow - perhaps using data- attributes?

@rgaudin
Copy link
Member

rgaudin commented Jan 4, 2022

Would you be able to submit a PR ?

What about:

gutenberg/gutenbergtozim/templates/cover_article.html

Line 20 in cb6a16c

<p class="label-value"{{ translate_author}}><a href='javascript:goToAuthor("{{ book.author.name() }}");'>{{ book.author.name() }}</a></p>

Also, it looks like there's an unused <script> in bookshelf.html.

@Jaifroid
Copy link
Author

Jaifroid commented Jan 4, 2022

The base.html template fills the onload attribute this way: onload="init(); {% if show_books %} showBooks(); {% else %} populateFilters(); {% endif %} {%if bookshelf_home %} showBookshelfSearchResults(''); {%endif%} {%if bookshelf is defined%} showBookshelf({{ bookshelf }} ); {%endif%}

@eshellman Thanks for the quick reply. Are these templates that are filled at build time? Or are they processed client-side?

@Jaifroid
Copy link
Author

Jaifroid commented Jan 4, 2022

@rgaudin That looks a bit like ReactJS to me, but it might be some other templating system... We realize it may not be possible to eliminate all inline scripting for ZIMs that rely heavily on custom UIs, especially those made with React. But if we can ensure a base level of accessibility for clients with restrictive CSPs, it would be really helpful.

@rgaudin
Copy link
Member

rgaudin commented Jan 4, 2022

I believe it's Jinja templates, rendered at build time. Fixing this onload seems doable. What about that link I mentioned above?

@Jaifroid
Copy link
Author

Jaifroid commented Jan 4, 2022

What about that link I mentioned above?

This looks like the inline link that users can click (from any book cover) to see more books by the same author. It's also blocked by the Chrome Extension CSP, so again that could be a link added from within one of the attached JS files. Code needed would be to give that anchor an ID (let's say authorName), and then:

document.getElementById('authorName').addEventListener('click', function(e) {
    let author = e.target.innerHTML;
    if (author) goToAuthor(author);
});

Code untested of course!

@mossroy mossroy mentioned this issue Jan 5, 2022
Closed
@stale
Copy link

stale bot commented Mar 30, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Mar 30, 2022
@mossroy mossroy mentioned this issue Jun 2, 2022
Closed
11 tasks
@stale stale bot removed the stale label Dec 17, 2022
@kelson42 kelson42 added this to the 1.2.0 milestone Dec 17, 2022
@kelson42 kelson42 pinned this issue Jan 25, 2023
@kelson42
Copy link
Contributor

With openedx an kolibri, this is the last of this kind in all our scrapers. Would be really great to have it fixes and allow KiwixJS to fully enjoy Gutenberg ZIM files.

@Jaifroid Jaifroid mentioned this issue Feb 23, 2023
Merged
@kelson42 kelson42 modified the milestones: 2.0.0, 2.1.0 Feb 26, 2023
@kelson42
Copy link
Contributor

This is a priority bug in case there is a doubt somewhere. Have pinned it.

@Jaifroid
Copy link
Author

Jaifroid commented Apr 16, 2023 •
edited

Just to say that. having experimentally (and successfully) ported the Kiwix JS chromium extension to the next-generation manifest v3 format,, the CSP block on unsafe-inline scripts (and unsafe-eval) continues to be highly relevant. This issue isn't going away, and I suspect it will become more generalized as security-conscious CSPs become more widespread.

This was referenced Apr 20, 2023
Closed
Open
@benoit74 benoit74 self-assigned this Jul 4, 2023
@benoit74 benoit74 mentioned this issue Aug 14, 2023
Merged
@kelson42 kelson42 unpinned this issue Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@benoit74 benoit74

Labels
enhancement good first issue
Projects
None yet
Milestone
2.1.0
Development

Successfully merging a pull request may close this issue.

Migrate to new PG server + remove inline JS + remove JS deps from repo openzim/gutenberg
5 participants
@rgaudin @eshellman @kelson42 @Jaifroid @benoit74