https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38365
--- Comment #253 from Jonathan Druart <[email protected]> --- (In reply to David Cook from comment #237) > 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. See "Bug 38365: Add xt test" > > 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. Done. > > 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. Todo. > > 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 Done. > > 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. Done. > > 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? Todo. > > 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. Done. > > 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? No idea! There is nothing else! Don't you see it? > > 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. Done. > > 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). Ok, however I found more: Caught by the xt tests (and now by add_csp_nonces.pl as well) * t/lib/plugins/Koha/Plugin/TestValuebuilder/test_valuebuilder_popup.tt And those ones when testing the UI: * cataloguing/value_builder/* (oops, all cataloguing plugins are broken) We might need to adjust the xt test and add_csp_nonces.pl to caught those files as well. Also wondering if this one won't require adjustment: misc/devel/tidy.pl:172 $content =~ s#\n*( *)<script>\n*#\n$1<script>\n#g; > > 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? Looks like it is used to load all the inline style tags. Todo - Vue apps are broken. Todo - Adjust xt test to parse .vue files(?) > > 11. Not blocker but add_csp_nonces.pl should use Koha::Devel::Files to list > > files. > > Could you elaborate on that? See "Bug 38365: Use Koha::Devel::Files->ls_tt_files in add_csp_nonces.pl" > > 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). Ok, will try again later, when the other points will be resolved. > > 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. If this one passes first it will be messy, but I am inclined to do the dirty job. -- 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/
