https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38365 --- Comment #356 from David Cook <dcook@prosentient.com.au> --- (In reply to Lari Taskula from comment #325)
(In reply to David Cook from comment #315)
Created attachment 194607 [details] [review] [review] Bug 38365: Fix t/db_dependent/Koha/Plugins/Valuebuilder_hooks.t
This initialises the CSP nonce in the unit test so that t/lib/plugins/Koha/Plugin/TestValuebuilder/test_valuebuilder_popup.tt will get its script element rewritten correctly.
Do we still need this patch after excluding test_valuebuilder_popup.tt in Koha::Devel::Files and xt/find-missing-nonce.t?
Yes. It's used by t/lib/plugins/Koha/Plugin/TestValuebuilder.pm
Is there a way to make Valuebuilder_hooks.t test fail in the first place? I tried rebasing before and after this commit but it always passed, so I don't understand the purpose of this change and the patch.
Yes, if you comment out these lines: use Koha::ContentSecurityPolicy; my $csp = Koha::ContentSecurityPolicy->new; $csp->set_nonce(); Then it will fail. There must've been an issue with your rebase I'm guessing.
`add_csp_nonces.pl --apply` still modifies test_valuebuilder_popup.tt. Should we call its Koha::Devel::Files constructor with context => 'nonce' so that test_valuebuilder_popup.tt will not be modified by the script?
Good catch. I'll fix that.
(In reply to David Cook from comment #316)
Created attachment 194608 [details] [review] [review] Bug 38365: Make tidy.pl compatible with script elements with attributes
tidy.pl will still tidy whitespace around script elements that have attributes like "nonce"
OK. `perl misc/devel/tidy.pl --tt` changes t/mock_templates/opac-tmpl/bootstrap/en/modules/opac-csp.tt (file introduced in this bug).
(In reply to David Cook from comment #317)
Created attachment 194609 [details] [review] [review] Bug 38365: Set __webpack_nonce__ for style-loader plugin and fix fontawesome imports This is out of my expertise, how to test?
My bad. I thought that I wrote a test plan for it. You need to rebuild the resources, turn on ERM, and visit the ERM home. Steps: 1. perl build-resources.PL (this is part of the "important" test plan) 2. Enable syspref ERMModule 3. Go to http://localhost:8081/cgi-bin/koha/erm/home 4. Note that no CSP violations appear in the console
Not to take anything away from the good work and important findings on the staff client side as seen in that patch, but we should redefine the scope of this Bug. It is clear from the title of this Bug [1] that the scope has been expanding. It has also been established that CSP is not ready to be enabled in the staff client and my understanding is that a lot of work remains to be done there. But as far as we know OPAC is quite close. So my question is how much do we want to (and can) work on the staff client side of things in this Bug?
We have a library that is willing to enable this on the OPAC side. In fact they are already running an older implementation but we would like to see it replaced with this one. Having this tested in the real world would be good even if it was initially just OPAC.
I also fear that we might run out of steam if this Bug's scope expands too much into the staff client as well.
[1] (at the time of this comment the title is "Add Content-Security-Policy HTTP header to HTML responses")
Yeah, I think this is as far as we go for bug 38365. We sign this off, QA it, and push it as is. -- Once bug 38365 is pushed, we can require that people add nonces to <script> and <style> inline elements (the QA tools will alert to this). And then I'll continue the work on bug 38407. Since the value builders are handled by Koha::FrameworkPlugin, that leaves only about 117 lines across 33 files, which isn't too bad at all. (But I agree is out of scope for bug 38365, which is big enough already.) -- You are receiving this mail because: You are watching all bug changes.