It would be nice if we could define additional MARC fields in a system preference via Template Toolkit and then have them display on the results/detail pages.
Created attachment 182054 [details] [review] Bug 39860: Add ExtraContentForXSLTDisplay system preference
Created attachment 182055 [details] [review] Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new 'ExtraContentForXSLTDisplay' system preference 3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a ) 4. Add the following to the ExtraContentForXSLTDisplay sys pref: [% IF record.subfield('538' , 'a') %] <span class="results_summary test1"> <span class="label">The 538a: </span> [% record.subfield('538' , 'a') %] </span> [% END %] 5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
Created attachment 182056 [details] [review] Bug 39860: Add ExtraContentForXSLTDisplay system preference
Created attachment 182057 [details] [review] Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new 'ExtraContentForXSLTDisplay' system preference 3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a ) 4. Add the following to the ExtraContentForXSLTDisplay sys pref: [% IF record.subfield('538' , 'a') %] <span class="results_summary test1"> <span class="label">The 538a: </span> [% record.subfield('538' , 'a') %] </span> [% END %] 5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
Created attachment 182058 [details] [review] Bug 39860: Use HTML scrubber to sanitize HTML
Still some problems: -Needs unit tests -I tried using HTML scrubber to scrub script tags but it scrubs too much. I want to be able to use most HTML tags, maybe just not JS?
Created attachment 182059 [details] [review] Bug 39860: Fix scrubbing and handling of HTML/TT in MARC21 xsl
This was helpful: https://stackoverflow.com/questions/5268182/how-to-remove-namespaces-from-xml-using-xslt
For repeating fields: [% IF record.subfield('538' , 'a') %] <span class="results_summary test1"> <span class="label">The 538a: </span> [% FOREACH f IN record.field('538') %] [% f.subfield('a') %] [% END %] </span> [% END %]
I've got a few comments: 1. It looks like you figured out the profile/settings argument for C4::Scrubber in the end, but I'd argue that "staff" is too permissive. One could argue since it's a system preference an admin user could already inject malicious JS via *UserJS, but I suppose the danger here is the XSS coming from the MARC record without the admin user's knowledge. I think we'd need a more specific profile/setting for scrubbing this content. I had a bug report somewhere that allowed for scrubber settings that could be deployed locally for maximum flexibility, but I can't find it right now... Failing QA for this one. 2. Why would we need to run this through the XSLTs? Wouldn't it be more efficient add it to the return data of C4::XSLT::XSLTParse4Display()? (That is, right now we return the return value of $engine->transform(), but we could store that, append to it, and then return that concatenated value. You might have a good reason for not doing that, but I don't see what it is at a glance on my own.) 3. Technically, we don't use XHTML anymore - we use HTML5 which isn't fully XML-compatible, so in theory someone could enter valid HTML which might cause XSLT errors. I'd be curious how it handles HTML5's "<br>" instead of XHTML's "<br/>" for instance. I wonder if it would croak on there not being a closing tag. 4. You shouldn't use Koha::TemplateUtils::process_tt() here. I'll DM you with the reason why. Also Failed QA for this one.
(In reply to Lucas Gass (lukeg) from comment #6) > -I tried using HTML scrubber to scrub script tags but it scrubs too much. I > want to be able to use most HTML tags, maybe just not JS? In practice, this is actually pretty challenging to do. The obvious one is to restrict <script> tags, but there's lots of other ways of injecting Javascript via other tags and attributes. (I should compile a list one of these days, as it's difficult to keep track of them all, but that's also part of the problem with a list... maintenance of the list.) Anyway, not going to give away all my security secrets here, but just... yeah it's challenging balancing security and convenience/flexibility.
Note that once we apply Content-Security-Policy, we'll have a great broad layer that prevents XSS, although there's still other malicious things people could do beyond XSS.
That all said, I do think this is an interesting idea! In fact, generally speaking, it's a direction that I'm quite keen to explore! I like the ideas of libraries being able to have more direct control over the display of their data. I just want to make sure we do it in a secure way.
Disclaimer: I didn't read David's longer comment, but stumbled on the pref name: ExtraContentForXSLTDisplay. It made me think I would have to add XSLT, maybe we could use ExtraContentForDetailPage or similar instead?
(In reply to Katrin Fischer from comment #14) > Disclaimer: I didn't read David's longer comment The short version is mostly "there are security problems" and a couple other potential gotchas.
Created attachment 182230 [details] [review] Bug 39860: Do not XSLT transform the custom content
> 2. > Why would we need to run this through the XSLTs? > > Wouldn't it be more efficient add it to the return data of > C4::XSLT::XSLTParse4Display()? > > (That is, right now we return the return value of $engine->transform(), but > we could store that, append to it, and then return that concatenated value. > You might have a good reason for not doing that, but I don't see what it is > at a glance on my own.) You're right, no need to do that. I wrote a follow up to append the content you like suggest, after the XSLT transformation. > 3. > Technically, we don't use XHTML anymore - we use HTML5 which isn't fully > XML-compatible, so in theory someone could enter valid HTML which might > cause XSLT errors. I'd be curious how it handles HTML5's "<br>" instead of > XHTML's "<br/>" for instance. I wonder if it would croak on there not being > a closing tag. This shouldn't be a problem now with the follow up patch not running through the XSLT transformation.
Created attachment 182231 [details] [review] Bug 39860: Do not XSLT transform the custom content
Created attachment 182232 [details] [review] Bug 39860: Add ExtraContentForXSLTDisplay system preference
Created attachment 182233 [details] [review] Bug 39860: Allow for the defining of custom MARC fields on the staff interface record display pages To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new 'ExtraContentForXSLTDisplay' system preference 3. Find or create a record that has some MARC fields that don't show up by default in the record display. ( I tested using a 538$a ) 4. Add the following to the ExtraContentForXSLTDisplay sys pref: [% IF record.subfield('538' , 'a') %] <span class="results_summary test1"> <span class="label">The 538a: </span> [% record.subfield('538' , 'a') %] </span> [% END %] 5. Go to the record from step 3, you should see the 538$a labeled as 'The 538a:' with the appropriate data in it.
This is interesting, I'll have to look how the xslt changes our libraries want would work with this ...
Could it be possible to pass the user's selected UI/intranet language so we could build translations based on it like: [% IF record.subfield('020' , 'q') %] [% IF lang == 'fi-FI' %] <span class="results_summary test1"> <span class="label">Sidosasu: </span> [% record.subfield('020' , 'q') %] </span> [% ELSIF lang == 'en' %] ... I tried to pass it as a variable in the XSLT.pm module but couldn't get it to show and the lang TT parameter used in other parts of Koha templates was not available. Could be that I'm not using the TT correctly. It feels like we can't use: [% USE AuthorisedValues %] [% USE Languages %] etc. either but my skills with template toolkit end here :).
Our specific need would be to translate codes you get from 942c ( Koha [default] item type ) to a readable form with translations for them.
(In reply to Lari Strand from comment #23) > Our specific need would be to translate codes you get from 942c ( Koha > [default] item type ) to a readable form with translations for them. (In reply to Lari Strand from comment #22) > Could it be possible to pass the user's selected UI/intranet language so we > could build translations based on it like: > > [% IF record.subfield('020' , 'q') %] > [% IF lang == 'fi-FI' %] > <span class="results_summary test1"> > <span class="label">Sidosasu: </span> > [% record.subfield('020' , 'q') %] > </span> > [% ELSIF lang == 'en' %] > ... > > I tried to pass it as a variable in the XSLT.pm module but couldn't get it > to show and the lang TT parameter used in other parts of Koha templates was > not available. Could be that I'm not using the TT correctly. It feels like > we can't use: > > [% USE AuthorisedValues %] > [% USE Languages %] > > etc. either but my skills with template toolkit end here :). Translations are a good point, I think the way to deal with that is probably to move this from a system preference to something like an HTML customization/additional contents.
(In reply to Lucas Gass (lukeg) from comment #24) > Translations are a good point, I think the way to deal with that is probably > to move this from a system preference to something like an HTML > customization/additional contents. Yeah I reckon that's probably the way to do it.