Bug 12758 - Failure when loading or parsing XSLT stylesheets over HTTPS
Summary: Failure when loading or parsing XSLT stylesheets over HTTPS
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: dependency
: 25216 (view as bug list)
Depends on: 12973
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-14 08:13 UTC by Marcel de Rooy
Modified: 2023-06-08 22:28 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 12758: Workaround for loading stylesheets over https (2.37 KB, patch)
2014-10-02 12:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Workaround for loading stylesheets over https (2.58 KB, patch)
2014-10-02 12:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Make LWP::Protocol::https required module (1.30 KB, patch)
2022-06-29 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Introduce Koha::XSLT::Loader (2.56 KB, patch)
2022-06-29 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Add Loader call in Koha::XSLT::Base (1.98 KB, patch)
2022-06-29 12:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Make LWP::Protocol::https required module (1.30 KB, patch)
2022-07-28 11:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Introduce Koha::XSLT::HTTPS (7.27 KB, patch)
2022-07-28 11:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Add new module call in Koha::XSLT::Base (2.14 KB, patch)
2022-07-28 11:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12758: Make LWP::Protocol::https required module (1.30 KB, patch)
2022-09-20 15:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 12758: Introduce Koha::XSLT::HTTPS (7.28 KB, patch)
2022-09-20 15:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 12758: Add new module call in Koha::XSLT::Base (2.15 KB, patch)
2022-09-20 15:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 12758: Make LWP::Protocol::https required module (1.35 KB, patch)
2022-10-28 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12758: Make LWP::Protocol::https required module (1.35 KB, patch)
2022-10-28 14:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12758: Introduce Koha::XSLT::HTTPS (7.34 KB, patch)
2022-10-28 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12758: Add new module call in Koha::XSLT::Base (2.21 KB, patch)
2022-10-28 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12758: (QA follow-up) Enhance POD (1.43 KB, patch)
2022-11-09 12:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2014-08-14 08:13:57 UTC

    
Comment 1 Marcel de Rooy 2014-08-14 08:14:35 UTC
See also IRC discussion on August 14, 2014
Comment 2 Marcel de Rooy 2014-08-14 08:18:03 UTC
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..
Comment 3 Marcel de Rooy 2014-08-14 08:29:43 UTC
Have sent an email to the cpan author of LibXML..
Comment 4 Marcel de Rooy 2014-10-02 11:46:25 UTC
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.
Comment 5 Marcel de Rooy 2014-10-02 12:13:21 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2014-10-02 12:24:03 UTC
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.
Comment 7 Marcel de Rooy 2014-11-17 08:05:52 UTC
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.
Comment 8 Katrin Fischer 2021-12-12 13:08:49 UTC
*** Bug 25216 has been marked as a duplicate of this bug. ***
Comment 9 Lucas Gass 2022-06-28 20:58:31 UTC
Any chance of getting this moving again? It makes using a external stylesheet nearly impossible in 2022, as most things are https.
Comment 10 Martin Renvoize 2022-06-29 05:55:13 UTC
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.
Comment 11 Marcel de Rooy 2022-06-29 11:16:20 UTC
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..
Comment 12 Marcel de Rooy 2022-06-29 12:18:34 UTC
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>
Comment 13 Marcel de Rooy 2022-06-29 12:18:38 UTC
Created attachment 136712 [details] [review]
Bug 12758: Introduce Koha::XSLT::Loader

Work in progress. This is the idea for a 'solution' ;)
Comment 14 Marcel de Rooy 2022-06-29 12:18:42 UTC
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.
Comment 15 Marcel de Rooy 2022-06-29 12:19:43 UTC
This is just a rough sketch of how to address it.
The Loader should be able to handle nested https includes.. Recursion.
Comment 16 Marcel de Rooy 2022-06-29 12:24:19 UTC
Will be continued later on.
Comment 17 Martin Renvoize 2022-06-29 13:34:15 UTC
Good call Marcel, thanks for taking a look.
Comment 18 Marcel de Rooy 2022-07-01 09:23:46 UTC
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 );
}
Comment 19 Marcel de Rooy 2022-07-01 09:25:39 UTC
So let me stick to the original partial approach and only fix the https ref at the file level
Comment 20 Lucas Gass 2022-07-19 17:11:05 UTC
Marcel,

Is this ready for testing?
Comment 21 Marcel de Rooy 2022-07-20 11:03:29 UTC
(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.
Comment 22 Marcel de Rooy 2022-07-28 11:08:37 UTC
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>
Comment 23 Marcel de Rooy 2022-07-28 11:08:42 UTC
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>
Comment 24 Marcel de Rooy 2022-07-28 11:08:46 UTC
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>
Comment 25 Marcel de Rooy 2022-07-28 11:09:56 UTC
(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?
Comment 26 Marcel de Rooy 2022-09-06 13:09:43 UTC
(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
Comment 27 Marcel de Rooy 2022-09-15 05:45:11 UTC
Still giving this development a few days, but I will close it if no one is interested after all.
Comment 28 David Nind 2022-09-16 13:55:11 UTC
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.
Comment 29 Marcel de Rooy 2022-09-20 13:24:31 UTC
(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 /
Comment 30 David Nind 2022-09-20 15:19:08 UTC
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>
Comment 31 David Nind 2022-09-20 15:19:13 UTC
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>
Comment 32 David Nind 2022-09-20 15:19:17 UTC
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>
Comment 33 David Nind 2022-09-20 15:43:22 UTC
(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!
Comment 34 Marcel de Rooy 2022-09-21 11:09:43 UTC
(In reply to David Nind from comment #32)
> Signed-off-by: David Nind <david@davidnind.com>

Great
Comment 35 Kyle M Hall 2022-10-28 14:32:09 UTC
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>
Comment 36 Kyle M Hall 2022-10-28 14:34:55 UTC
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>
Comment 37 Kyle M Hall 2022-10-28 14:35:12 UTC
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>
Comment 38 Kyle M Hall 2022-10-28 14:35:16 UTC
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>
Comment 39 Tomás Cohen Arazi 2022-11-07 13:48:23 UTC
Mason, can you confirm there are no dependency issues?
Comment 40 Mason James 2022-11-08 21:07:26 UTC
(In reply to Tomás Cohen Arazi from comment #39)
> Mason, can you confirm there are no dependency issues?

looks good to me!
Comment 41 Tomás Cohen Arazi 2022-11-09 12:59:48 UTC
Created attachment 143504 [details] [review]
Bug 12758: (QA follow-up) Enhance POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2022-11-09 13:06:29 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!