See also IRC discussion on August 14, 2014
Just to narrow down the problem: You can use https:etc.xsl in load_xml, but if you include an import href="https:utils.xsl" in that etc.xsl, it fails to load. Seems to be a problem of XML::LibXML, but would be nice to resolve it one way or the other..
Have sent an email to the cpan author of LibXML..
I have renamed this report to widen its scope. Actually, I see here two related problems: 1) LibXML does not like https references when loading xml with the location parameter. This can be resolved by reading the file into a string and passing it to load_xml via the string parameter. (A solution within LibXML for https appears not to be expected for now.) Note btw bthat reading the file might not be working rightaway. If you LWP::UserAgent to get the file contents, it will need LWP::Protocol::https. This dependency was recently added to Koha as optional in the context of OverDrive integration. Should we mark it as required now? 2) If the stylesheet (via local file, http or https) contains an import with a https reference, LibXML can load it (possibly with the workaround under point 1), but LibXSLT will now fail parsing it with: "I/O warning : failed to load external entity". This is harder to resolve; we should not be replacing imports in xslt code ourselves. The problem seems to go down deeper to the libxml2 C library.
Created attachment 31974 [details] [review] Bug 12758: Workaround for loading stylesheets over https LibXML does not like https references when loading xml with the location parameter. This can be resolved by reading the file into a string and passing it to load_xml via the string parameter. Note that LWP::UserAgent will only load the file if LWP::Protocol::https is installed. This is an optional perl dependency in Koha. I mark it as required now. Also note that if your stylesheet contains an import with a https reference, this fix is not enough. The load_xml call will not fail, but the succeeding call to parse_stylesheet will still fail. As a workaround, make your import file accessible via http. NOTE: Some code taken from report 12172. With thanks to Martin Renvoize.
Created attachment 31975 [details] [review] Bug 12758: Workaround for loading stylesheets over https LibXML does not like https references when loading xml with the location parameter. This can be resolved by reading the file into a string and passing it to load_xml via the string parameter. Note that LWP::UserAgent will only load the file if LWP::Protocol::https is installed. This is an optional perl dependency in Koha. I mark it as required now. Also note that if your stylesheet contains an import with a https reference, this fix is not enough. The load_xml call will not fail, but the succeeding call to parse_stylesheet will still fail. As a workaround, make your import file accessible via http. NOTE: Some code taken from report 12172. With thanks to Martin Renvoize. Test plan: Take for example OPACXSLTDetailsDisplay and replace it by a https ref. Check opac detail display. Note that replacing OPACXSLTResultsDisplay by some https ref would also test repeating the call.
Changing the status of this report to an arbitrary Failed QA. I do not really like the workaround; it does pollute the code. But it works if you look for a workaround for this problem. A solution at the level of LibXML/LibXSLT would be much better. The Failed QA status still allows this report to be found when 'google' searching in Bugzilla. Only change the status please, if you have a good reason for doing so. Thanks.
*** Bug 25216 has been marked as a duplicate of this bug. ***
Any chance of getting this moving again? It makes using a external stylesheet nearly impossible in 2022, as most things are https.
Wow, I knew I recognised this one.. I reported it upstream many years ago: https://rt.cpan.org/Public/Bug/Display.html?id=98024 Looking at the discussions linked think there is looking less and less likely that such support will get baked into libxml2 upstream.. in fact it looks like they're actually considering the removal of the http client stuff entirely at some point and they strongly recommend against loading over remote resources for performance reasons. As such, I think we can and should follow Marcels 'hack' here.
Its a patch from 2014 from me. I kept it in FQA for reference. The way I am looking now against it, I would rather put some module in between to do the actual workaround in the (little) hope that LibXML would be resolved some day. I might give it a chance, no promises..
Created attachment 136711 [details] [review] Bug 12758: Make LWP::Protocol::https required module Moving from recommends to required in cpanfile. Reference info: Debian package liblwp-protocol-https-perl is available for Buster (6.07-2) and Bullseye (6.10-1). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 136712 [details] [review] Bug 12758: Introduce Koha::XSLT::Loader Work in progress. This is the idea for a 'solution' ;)
Created attachment 136713 [details] [review] Bug 12758: Add Loader call in Koha::XSLT::Base Reviving a report back from 2014. The workaround is now moved to a separate helper module from another patch. Test plan: Test OPACXSLTDetailsDisplay with an https reference. Test it again with a stylesheet referring to a https include.
This is just a rough sketch of how to address it. The Loader should be able to handle nested https includes.. Recursion.
Will be continued later on.
Good call Marcel, thanks for taking a look.
My plan was somehting like this (unfinished on itself): Still had to set the flag when I found a https ref. And check for commented refs. But this is just way too much imo. sub _convert_https { my ( $self, $code, $key, $params ) = @_; my $level = $params->{level} // 0; # to limit recursion level if( $level > $self->max_recursion_level ) { die; #FIXME Exception } # Find refs and resolve recursively my ( $href, $tmp_name, $new_code, $flag, @replaces, $changed ); while( $code =~ /<(?:xsl):(?:import|include)\s*href\s*=\s*['"]([^'"]+)['"]/gcs ) { $href = $1; if( $include_cache->{$href} ) { $new_code = $include_cache->{$href}; $changed = 1; } else { $new_code = $self->_get_code( $href ); ( $new_code, $flag ) = $self->_convert_https( $new_code, $href, { level => $level + 1 } ); $changed = 1 if $flag; } next unless $changed; $tmpname = MIME::Base64::encode_base64url( $href, q{} ); if( !$self->{_tempdir} || !-e $self->{_tempdir}. '/'. $tmpname ) { $self->{_tempdir} //= File::Temp->newdir; write_file( $self->{_tempdir}. '/'. $tmpname, { binmode => ':utf8' }, Encode::encode_utf8($new_code) ); } push @replaces, { href => $href, file => $self->{_tempdir}. '/'. $tmpname }; } # Replace refs foreach my $h ( @replaces ) { $code =~ s/(?=<(?:xsl):(?:import|include)\s*href\s*=\s*)(['"])${h->{href}}['"]/$1${h->{file}}$1/; } # Finalize $include_cache->{$key} = $code if !$self->disable_caching && $changed && $key; return ( $code, $changed // 0 ); }
So let me stick to the original partial approach and only fix the https ref at the file level
Marcel, Is this ready for testing?
(In reply to Lucas Gass from comment #20) > Marcel, > > Is this ready for testing? No, not yet. I still have a unfinished development for it.
Created attachment 138199 [details] [review] Bug 12758: Make LWP::Protocol::https required module Moving from recommends to required in cpanfile. Reference info: Debian package liblwp-protocol-https-perl is available for Buster (6.07-2) and Bullseye (6.10-1). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 138200 [details] [review] Bug 12758: Introduce Koha::XSLT::HTTPS This module now fetches a https stylesheet and returns it as a string to Base. (As a workaround waiting for a real solution in the underlying libraries.) The module can be extended to resolve includes but this requires parsing the xslt code. Test plan: [1] Run t/Koha_XSLT_HTTPS.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 138201 [details] [review] Bug 12758: Add new module call in Koha::XSLT::Base Reviving a report back from 2014. The workaround is now moved to a separate helper module from another patch. Test plan: Test OPACXSLTDetailsDisplay with an https reference. You may try: https://library.rijksmuseum.nl/opac-tmpl/bootstrap/en/xslt/zztest.xsl If you flush and restart, the opac detail pages will contain the text 'This is my MARC record.'. NOTE: If you point to an xslt file over https that contains includes or imports, you will still get the error: Failed to load external entity. To address that, we need to extend the _resolve_includes sub. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Lucas Gass from comment #20) > Marcel, > > Is this ready for testing? Lukas, the current patches can be tested. They will only resolve pure https references without includes or imports. That would need further work and could be done on a new report?
(In reply to Marcel de Rooy from comment #25) > (In reply to Lucas Gass from comment #20) > > Marcel, > > > > Is this ready for testing? > > Lukas, the current patches can be tested. They will only resolve pure https > references without includes or imports. > That would need further work and could be done on a new report? Ping Lucas
Still giving this development a few days, but I will close it if no one is interested after all.
I'm getting: - A blank record in the OPAC - This error in the logs: [2022/09/16 11:07:11] [WARN] XSLT::Base: Exception 'Koha::Exceptions::XSLT::FetchFailed' thrown 'Fetching xslt file failed' I think that is because https://library.rijksmuseum.nl/opac-tmpl/bootstrap/en/xslt/zztest.xsl is not accessible The tests pass.
(In reply to David Nind from comment #28) > I think that is because > https://library.rijksmuseum.nl/opac-tmpl/bootstrap/en/xslt/zztest.xsl is not > accessible Ah sorry. Yes that was probably gone with the wind :) Temporary file in a container that got replaced or so. Here is another (temporary) link. It works now and at least until monday: https://prod-intra.rijkskoha.nl/bug12758.xsl Use it for Results XSL. Showing something like this in results display: What a great title: Learning Perl /
Created attachment 140790 [details] [review] Bug 12758: Make LWP::Protocol::https required module Moving from recommends to required in cpanfile. Reference info: Debian package liblwp-protocol-https-perl is available for Buster (6.07-2) and Bullseye (6.10-1). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 140791 [details] [review] Bug 12758: Introduce Koha::XSLT::HTTPS This module now fetches a https stylesheet and returns it as a string to Base. (As a workaround waiting for a real solution in the underlying libraries.) The module can be extended to resolve includes but this requires parsing the xslt code. Test plan: [1] Run t/Koha_XSLT_HTTPS.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 140792 [details] [review] Bug 12758: Add new module call in Koha::XSLT::Base Reviving a report back from 2014. The workaround is now moved to a separate helper module from another patch. Test plan: Test OPACXSLTDetailsDisplay with an https reference. You may try: https://library.rijksmuseum.nl/opac-tmpl/bootstrap/en/xslt/zztest.xsl If you flush and restart, the opac detail pages will contain the text 'This is my MARC record.'. NOTE: If you point to an xslt file over https that contains includes or imports, you will still get the error: Failed to load external entity. To address that, we need to extend the _resolve_includes sub. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
(In reply to Marcel de Rooy from comment #29) > Here is another (temporary) link. It works now and at least until monday: > https://prod-intra.rijkskoha.nl/bug12758.xsl > Use it for Results XSL. Thanks Marcel!
(In reply to David Nind from comment #32) > Signed-off-by: David Nind <david@davidnind.com> Great
Created attachment 142768 [details] [review] Bug 12758: Make LWP::Protocol::https required module Moving from recommends to required in cpanfile. Reference info: Debian package liblwp-protocol-https-perl is available for Buster (6.07-2) and Bullseye (6.10-1). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 142770 [details] [review] Bug 12758: Make LWP::Protocol::https required module Moving from recommends to required in cpanfile. Reference info: Debian package liblwp-protocol-https-perl is available for Buster (6.07-2) and Bullseye (6.10-1). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 142771 [details] [review] Bug 12758: Introduce Koha::XSLT::HTTPS This module now fetches a https stylesheet and returns it as a string to Base. (As a workaround waiting for a real solution in the underlying libraries.) The module can be extended to resolve includes but this requires parsing the xslt code. Test plan: [1] Run t/Koha_XSLT_HTTPS.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 142772 [details] [review] Bug 12758: Add new module call in Koha::XSLT::Base Reviving a report back from 2014. The workaround is now moved to a separate helper module from another patch. Test plan: Test OPACXSLTDetailsDisplay with an https reference. You may try: https://library.rijksmuseum.nl/opac-tmpl/bootstrap/en/xslt/zztest.xsl If you flush and restart, the opac detail pages will contain the text 'This is my MARC record.'. NOTE: If you point to an xslt file over https that contains includes or imports, you will still get the error: Failed to load external entity. To address that, we need to extend the _resolve_includes sub. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Mason, can you confirm there are no dependency issues?
(In reply to Tomás Cohen Arazi from comment #39) > Mason, can you confirm there are no dependency issues? looks good to me!
Created attachment 143504 [details] [review] Bug 12758: (QA follow-up) Enhance POD Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!