Summary: | Removing remote include in MODS xslt | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | OPAC | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Ian Walls <koha.sekjal> |
Severity: | minor | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | chris, paul.poulain, semarie |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Patch
Revised patch Revised patch Rebased patch Bug 6803: Replacing remote XSL include |
Description
Marcel de Rooy
2011-08-29 09:13:09 UTC
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 |