https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38365
--- Comment #237 from David Cook <[email protected]> --- Thanks so much for looking at this, Jonathan! (In reply to Jonathan Druart from comment #232) > 1. I think we need a xt test to parse template files and catch regressions > from Jenkins. Let me know if I miss something, I can write it if you want me > to. That would be great if you could write it. Martin mentioned doing a xt test, but I didn't understand what he (or you) mean by it. I'm sure I'll understand once I see it. > 2. `use strict; use warnings;` should be replaced by `use Modern::Perl;` I can fix that up in t/Koha/Middleware/ContentSecurityPolicy.t and t/db_dependent/Koha/Middleware/ContentSecurityPolicy.t, which I think are the only two occurrences. > 3. Is there a good reason to use Plack::Test in > t/db_dependent/Koha/Middleware/ContentSecurityPolicy.t? IMO we should have a > Cypress test. > By the way I am expecting t/Koha/Middleware/ContentSecurityPolicy.t to fail > if there is no DB, why is it not in db_dependent? > Also I don't think those 2 includes are needed: > 27 use File::Basename qw(dirname); > 28 use HTTP::Request::Common; I'll let Lari followup with that one. > 4. Koha::Cache::Memory::Lite; is loaded in several modules but not used. Ah yep that's from an old revision. I'll remove from Koha/ContentSecurityPolicy.pm and t/Koha/ContentSecurityPolicy.t > 5. We will need a commit for ktd to sync koha-conf.xml Is the file "files/templates/koha-conf-site.xml.in" in KTD? I can do a PR for that. > 6. xt/api.t is failing > xt/api.t .. 1/8 > # Failed test 'Validation exit code is 0' > # at xt/api.t line 103. > # got: '256' > # expected: '0' > # Validation failed. /paths/public/csp-reports/post/parameters/body has > an invalid type (array,object) > # Looks like you failed 1 test of 1. Hmm that's annoying. It works in practice but swagger-cli must not like it. Perhaps we do need separate endpoints for the different report syntaxes. @Lari: can you take care of that? > xt/api.t .. 3/8 > # Failed test 'The spec passes the swagger-cli validation' > # at xt/api.t line 108. > > # Failed test 'No tag errors in the spec' > # at xt/api.t line 130. > # Structures begin differing at: > # $got->[0] = 'post /public/csp-reports -> uses tag 'csp' not > present in top level list' > # $expected->[0] = Does not exist > post /public/csp-reports -> uses tag 'csp' not present in top level list I can look into that. > 7. When enabled, I see an error in the browser's console: > Content-Security-Policy: The page’s settings blocked the loading of a > resource (media-src) at data: because it violates the following directive: > “default-src 'self'” That's interesting. I'd like to hear more about this one. Were there additional details? I wonder if it had to do with audio when doing a checkout or something? > Might be related to the other error: > Source map error: request failed with status 404 > Resource URL: > http://dev-intra.localhost/intranet-tmpl/lib/bootstrap/bootstrap.bundle. > min_25.1200026.js > Source Map URL: bootstrap.bundle.min.js.map This is a normal error. When you have the dev tools open, it'll try to fetch the non-existence bootstrap.bundle.min.js.map file. You can safely ignore that one. > > 8. It's not clear to me why we are using an env var, I would go with a > package variable. I actually faced the same problem a couple of weeks ago on > bug 41895, please have a look at Koha/Context/UserEnv.pm in the first patch > there. Originally, I was using the cache in the Plack Middleware, but the cache gets cleared by CGI::new at the start of CGI script handling. In theory a singleton using a package variable could work. I think I used an ENV variable because I knew it would be cleared with each HTTP request, but I suppose a package variable would too. Is it a blocker? Wouldn't be too hard to switch over to using a package variable. I do like the idea of using a package variable instead as it seems cleaner. > 9. Did you discuss those 3 occurrences? > misc/cronjobs/cloud-kw.pl:<style> > misc/cronjobs/overdue_notices.pl:<style type='text/css'> > misc/cronjobs/runreport.pl: > "<html><head><style>tr:nth-child(2n+1) { background-color: > #ccc;}</style></head><body>$message</body></html>"; No, we didn't talk about them. misc/cronjobs/overdue_notices.pl and misc/cronjobs/runreport.pl aren't relevant since they're sending HTML emails so CSP doesn't apply there. But misc/cronjobs/cloud-kw.pl is interesting. I think I've wanted to remove that cronjob for many years, but I think BibLibre still use it. At a glance... it looks like that HTML file is served directly from Apache rather than being generated by Plack, so it won't be covered by CSP, but it also can't really (unless a person adds the CSP header via Apache but I think that's out of scope for this change). > 10. Vue apps have styling issues: > Content-Security-Policy: The page’s settings blocked an inline style > (style-src-elem) from being applied because it violates the following > directive: “style-src 'self' 'nonce-UrNLygw_Cc1zaPU2WH0qVk'”. Consider using > a hash ('sha256-t4I2teZN5ZH+VM+XOiWlaPbsjQHe+k9d6viXPpKpNWA=', requires > 'unsafe-hashes' for style attributes) or a nonce. > It's coming from insertBySelector.js and styleTagTransform.js I have been wondering if the Vue apps would have trouble. According to https://webpack.js.org/loaders/style-loader/#nonce, style-loader isn't recommended for production. Note the security warning on https://www.npmjs.com/package/style-loader as well. How does style-loader get used in Vue currently? > 11. Not blocker but add_csp_nonces.pl should use Koha::Devel::Files to list > files. Could you elaborate on that? > 12. > (In reply to David Cook from comment #99) > > FYI bug 38407 will need to be resolved before the staff interface is ready > > for CSP in prod. > > There is no patch there, should this depends on it then? I don't think so. However, we probably should add a warning to koha-conf.xml saying not to enable CSP for intranet until after bug 38407 is resolved. Originally, I was only intending to add the CSP mechanism, and then add CSP to the OPAC, since it was ready for it. Scope creep has taken us much further. I'll still work on bug 38407 after we push this one, but I'm keen to at least get the mechanism into Koha. > 13. I didn't manage to see something in the violation logs. I enabled > dom.reporting.* in about:config, what else is needed? As Lari said, you need to have HTTPS enabled (you can do this using the snakeoil certs on the server), to use a domain name instead of local host, and make sure report-uri is set correctly because Firefox doesn't support "report-to" yet (but Chrome and Edge use "report-to" which uses a different syntax - ugh). > Side note: This will heavily conflict with bug 41324... Easy to fix, but > boring. I think it should be OK? We can toss the "Bug 38365: Add CSP nonces to all inline script and style tags" patch and re-generate to fix the merge conflict. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
