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.
Created attachment 5206 [details] [review] Patch
Test this patch by exporting to MODS format from opac-detail page. This patch does not change any functionality.
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.
(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.
Created attachment 5239 [details] [review] Revised patch Does no longer remove CRs. Note that the presence of these CRs will now give two [ignorable] whitespace errors when applying.
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.
(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.
With my git version, it does not seem to be possible to resolve this whitespace issue with carriage returns. I cannot send another patch.
Created attachment 5873 [details] [review] Revised patch In the meantime the xsl file has been converted to unix format. This resolves the whitespace trouble.
I still would like to have better test plan for this. Does this bug only happen when there is a connectivity issue with LOC?
(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.
(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.
(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).
Created attachment 6636 [details] [review] Rebased patch Now also includes nbsp entity def.
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>
QA comment * small patch, nothing specific to say * prove t/00-valid-xml.t is OK patch pushed, please test