Bug 6803 - Removing remote include in MODS xslt
Summary: Removing remote include in MODS xslt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) minor (vote)
Assignee: Marcel de Rooy
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-29 09:13 UTC by Marcel de Rooy
Modified: 2012-10-25 23:09 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (203.29 KB, patch)
2011-08-29 09:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Revised patch (8.88 KB, patch)
2011-08-31 12:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Revised patch (8.72 KB, patch)
2011-10-13 06:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Rebased patch (8.57 KB, patch)
2011-12-07 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6803: Replacing remote XSL include (8.58 KB, patch)
2011-12-10 09:53 UTC, Chris Cormack
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 2011-08-29 09:13:09 UTC
Removing remote include in MARC21slim2MODS3-1.xsl (http://www.loc.gov/marcxml/xslt/MARC21slimUtils.xsl).

Safer and preventing errors like:
[Sun Aug 28 04:47:39 2011] [error] [client 66.249.72.86] [Sun Aug 28 04:47:39 2011] opac-export.pl: compilation error: file http://www.loc.gov/shutdown-message.html line 2 element html
[Sun Aug 28 04:47:39 2011] [error] [client 66.249.72.86] [Sun Aug 28 04:47:39 2011] opac-export.pl: xsltParseStylesheetProcess : document is not a stylesheet
[Sun Aug 28 04:47:39 2011] [error] [client 66.249.72.86] [Sun Aug 28 04:47:39 2011] opac-export.pl:  at /usr/share/koha/prodclone/C4/Record.pm line 260

At the same time removing CRs from the MODS xsl file with dos2unix.
Comment 1 Marcel de Rooy 2011-08-29 09:15:36 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-08-29 09:16:48 UTC
Test this patch by exporting to MODS format from opac-detail page.
This patch does not change any functionality.
Comment 3 Owen Leonard 2011-08-29 13:37:17 UTC
Does this bug only happen when there is a connectivity issue with LOC? Just wondering how to reproduce the problem so I can verify the fix.

Also, why the CR removal? Was that causing problems? I would much rather see this patch only include the correction, and see the CR removal in a different patch if it's a necessary change.
Comment 4 Marcel de Rooy 2011-08-29 13:41:10 UTC
(In reply to comment #3)
> Does this bug only happen when there is a connectivity issue with LOC? Just
> wondering how to reproduce the problem so I can verify the fix.
> Also, why the CR removal? Was that causing problems? I would much rather see
> this patch only include the correction, and see the CR removal in a different
> patch if it's a necessary change.

Probably, LOC was down, rebooting or something like that. (This was again discovered by Googlebot..) Hard to reproduce.
I agree that the CR removal is not really necessary.
In terms of security it is probably safer to remove the remote include.
Comment 5 Marcel de Rooy 2011-08-31 12:57:08 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2011-08-31 13:34:11 UTC
Applying: 6803: Removing remote include in MODS xslt
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS3-1.xsl:1
error: koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS3-1.xsl: patch does not apply
/kohaclone/.git/rebase-apply/patch:31: new blank line at EOF.
Comment 7 Marcel de Rooy 2011-08-31 13:40:00 UTC
(In reply to comment #6)
> Applying: 6803: Removing remote include in MODS xslt
> error: patch failed:
> koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS3-1.xsl:1
> error: koha-tmpl/intranet-tmpl/prog/en/xslt/MARC21slim2MODS3-1.xsl: patch does
> not apply
> /kohaclone/.git/rebase-apply/patch:31: new blank line at EOF.

It applies with me but warning for whitespace (CRs ?).
This could be a git version difference.
Will try to remove them and send a new one.
Comment 8 Marcel de Rooy 2011-08-31 14:02:09 UTC
With my git version, it does not seem to be possible to resolve this whitespace issue with carriage returns. I cannot send another patch.
Comment 9 Marcel de Rooy 2011-10-13 06:55:03 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2011-11-04 19:27:34 UTC
I still would like to have better test plan for this. Does this bug only happen when there is a connectivity issue with LOC?
Comment 11 Sébastien Marie 2011-11-07 08:20:29 UTC
(In reply to comment #10)
> I still would like to have better test plan for this. Does this bug only happen
> when there is a connectivity issue with LOC?

You could modify the include in "MARC21slim2MODS3-1.xsl", in order to have the href point to a URI where the status in well-known (sorry not well explained... try with examples after...)

In order to obtain check with a connectivity issue:

 - for test a DNS problem: change in "MARC21slim2MODS3-1.xsl", the href (line 3) with : "http://www.loc.gov.example.com/marcxml/xslt/MARC21slimUtils.xsl"

This will result Koha trying resolving "www.loc.gov.example.com" (which should not exist), and see what happens.


 - for test a server error: (not exactly the same that before: DNS answered correctly, but no HTTP server found), replace by a valid DNS name where no HTTP server are running (for example "http://google-public-dns-a.google.com/marcxml/xslt/MARC21slimUtils.xsl", but you should prefer a local owned-host, instead of perform aggression to a public internet server)

This will result Koha successfully resolv "google-public-dns-a.google.com", but cannot connecting to http service.


 - for test a 404 error: replace by "http://www.loc.gov/marcxml/xslt/MARC21slimUtils.xsl.noexistant".

This will result Koha successfully resolv "www.loc.gov", successfully connect to http service, but (should) obtain a 404 error.
Comment 12 Marcel de Rooy 2011-11-07 08:33:34 UTC
(In reply to comment #11)
Thanks for more tests!
Do you think that is safer to replace this remote include by the way?

From another point of view (consistency), I would like to replace it too; it is the only remaining remote include in a xslt file in Koha.
Comment 13 Sébastien Marie 2011-11-07 09:19:08 UTC
(In reply to comment #12)
> Thanks for more tests!
> Do you think that is safer to replace this remote include by the way?
> 
> From another point of view (consistency), I would like to replace it too; it is
> the only remaining remote include in a xslt file in Koha.

It will be better to not depend of external source.

If I remember well about XSLT processing, XML::LibXSLT don't use security by default (and koha don't set it).

But, this remote inclusion is *not* a security issue (if you trust LOC), as for successfully use this vector, an attacker should:
 - or compromise LOC (change what it is included)
 - or compromise your local network infrastructure (DNS, router, server, ...)

So the risk is low (not null, but low).
Comment 14 Marcel de Rooy 2011-12-07 13:29:00 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2011-12-10 09:53:08 UTC
Created attachment 6698 [details] [review]
Bug 6803: Replacing remote XSL include

Replacing remote include by local one.
This resolves possible connectivity issues (see Bugzilla comments).
Should theoretically be safer and faster too.

December 7, 2011: Rebased and included nbsp entity definition (bug 7141).

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 16 Paul Poulain 2011-12-13 14:58:02 UTC
QA comment

* small patch, nothing specific to say
* prove t/00-valid-xml.t is OK

patch pushed, please test