Recently, we got a lot of security bugs related to potential XSS attack. We should think about a global solution instead of escaping everything manually in the templates. Any thoughs?
http://www.perlmonks.org/?node_id=1059961
Created attachment 35488 [details] [review] Bug 13618: [wip] Use Template::Stash::HTML::Entities to use the html filter Test plan: 0/ sudo cpanm Template::Stash::HTML::Entities 1/ Verify don't reproduce the XSS issue described on bug 13609.
Created attachment 35516 [details] [review] Revert "Bug 13609: Cross Site Scripting problem in authority search result list paging" This reverts commit c667b9ddbf42f9729d8f4035c7e872d5e980a5e9.
Hi Jonathan, is the second patch just for testing or supposed to go with this?
Jonathan, maybe we could think of resurrect some idea from bug 10075 and make it provide a escaped version of the params for direct usage?
(In reply to Katrin Fischer from comment #5) > Hi Jonathan, is the second patch just for testing or supposed to go with > this? It is part of the POC, all html filter will become useless in templates.
(In reply to Tomás Cohen Arazi from comment #6) > Jonathan, maybe we could think of resurrect some idea from bug 10075 and > make it provide a escaped version of the params for direct usage? I believe the solution I proposed is easier to implement, don't you think?
Interesting, I think its a good idea. One should consider the use do vars in JS code.
Chris has found Template::Stash::AutoEscape: http://search.cpan.org/~mala/Template-Stash-AutoEscape-0.03/lib/Template/Stash/AutoEscape.pm
and more active: Template::Stash::AutoEscaping http://search.cpan.org/~shlomif/Template-Stash-AutoEscaping-0.0303/lib/Template/Stash/AutoEscaping.pm
Perhaps we could use this module http://search.cpan.org/~shlomif/Template-Stash-AutoEscaping-0.0303/lib/Template/Stash/AutoEscaping.pm As it lets us do value.raw if we need unescaped values. But either we use it, or the HTML::Entities one, I think we should push it very soon. And we should do it on both the staff and opac sides
(In reply to Chris Cormack from comment #12) > Perhaps we could use this module > > http://search.cpan.org/~shlomif/Template-Stash-AutoEscaping-0.0303/lib/ > Template/Stash/AutoEscaping.pm > > As it lets us do value.raw if we need unescaped values. > > But either we use it, or the HTML::Entities one, I think we should push it > very soon. And we should do it on both the staff and opac sides +1 for Template::Stash::AutoEscaping
Created attachment 40480 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.)
Created attachment 40481 [details] [review] Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc
Created attachment 40482 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT.
Created attachment 40483 [details] [review] Bug 13618: Remove html filters at the intranet
Created attachment 40484 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra
Created attachment 40485 [details] [review] Bug 13618: Specific for pagination_bar
Created attachment 40487 [details] [review] Bug 13618: Specific for the ISBD view
html code are generated in C4::Reports::Guided::format_results but this subroutine will be removed by bug 14435.
Neato burrito! This could come in really really handy...
Jonathan: Is this ready for testing now?
(In reply to David Cook from comment #23) > Jonathan: Is this ready for testing now? yep :)
Would be great to get this tested, it will save us working on never ending individual fixes
I'm sorry, could you please rebase. There are several files in conflict right now. I'll give this priority this week, so I'll try to test this ASAP.
Created attachment 41346 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.)
Created attachment 41347 [details] [review] Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc
Created attachment 41348 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT.
Created attachment 41349 [details] [review] Bug 13618: Remove html filters at the intranet
Created attachment 41350 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra
Created attachment 41351 [details] [review] Bug 13618: Specific for pagination_bar
Created attachment 41352 [details] [review] Bug 13618: Specific for the ISBD view
Does not work: circulation.pl: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 618.
Created attachment 41353 [details] [review] Bug 13618: Fix error 'Not a GLOB reference' The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. The easier fix is to replace it with a SWITCH.
Found something else, the checkout table is broken on circulation.pl (JS errors).
Created attachment 41354 [details] [review] Bug 13618: Specific for IntranetUser* and OPACUser* prefs
Created attachment 41355 [details] [review] Bug 13618: Specific for ColumnsSettings
This patch seems to break sysprefs such as intranetnav and opacmainuserblock - they show their HTML instead of interpreting it. I haven't checked all of the user configurable areas with this patch but we definitely should check them all. Off the top of my head (ok, ok, I went and looked through, but I still might have missed some): opacmainuserblock opacnav opacnavright opaccredits opacheader opacuserjs opacusercss opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs intranetusercss intranetuserjs
Ok, sorry I didn't have the last couple of patches - that's my bad but we still should check all of those sysprefs as part of testing. :)
Actually, I'm sure I have all of the patches now, and intranetNav, at least, still has a problem. Also the news items are displaying their HTML, and they shouldn't be. LR
If it's any sys pref that deals with HTML/JS/CSS, here's a few more to check; what about all the notices/slips templates? Do those need to be checked, too, since they also use HTML/JS/CSS? Extra user-edited sys prefs: OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions
Created attachment 41746 [details] [review] Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error.
On memberentrygen.tt, all of the cases for *popup are broken, they show the html we're sending in that variable instead of the dropdown. Most of them are conditional, the main reason I noticed was because of the Patron title: field. Suspect there will be heaps of stuff like this.
Created attachment 41747 [details] [review] Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error.
Created attachment 41776 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions
Thanks for the pref list!
See https://github.com/joubu/Koha/tree/bug_13618 for an up-to-date patch set.
Created attachment 42009 [details] [review] Bug 13618: Specific for Salutation on editing a patron
Created attachment 42054 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan done and worked. I deleted the commits from 13609, and then applied this and it also solved the xss vulnerability. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
(In reply to Joonas Kylmälä from comment #51) > Created attachment 42054 [details] [review] [review] > Bug 13618: Use Template::Stash::AutoEscaping to use the html filter > > Test plan done and worked. I deleted the commits from 13609, and then > applied this and it also solved the xss vulnerability. > > Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Thought it worked, but now noticed it broked the html in /cgi-bin/koha/opac-tags.pl?mine=1 (my tags in opac). Under the title column it only shows html as plain text.
Created attachment 42072 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions
Created attachment 42073 [details] [review] Bug 13618: Specific for XSLTBloc
(In reply to Joonas Kylmälä from comment #52) > (In reply to Joonas Kylmälä from comment #51) > > Created attachment 42054 [details] [review] [review] [review] > > Bug 13618: Use Template::Stash::AutoEscaping to use the html filter > > > > Test plan done and worked. I deleted the commits from 13609, and then > > applied this and it also solved the xss vulnerability. > > > > Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> > > Thought it worked, but now noticed it broked the html in > /cgi-bin/koha/opac-tags.pl?mine=1 (my tags in opac). Under the title column > it only shows html as plain text. "Bug 13618: Specific for other prefs" reuploaded, OPACMySummaryNote was not correctly managed. New patch "Specific for XSLTBloc" should fix your issue
Created attachment 42075 [details] [review] Bug 13618: Specific for XSLTBloc Fixes the raw text nicely Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42078 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42079 [details] [review] Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. variables - intra
Created attachment 42087 [details] [review] Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42090 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42091 [details] [review] Bug 13618: Specific for Salutation on editing a patron Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42092 [details] [review] Bug 13618: Specific for the ISBD view Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
(In reply to Jonathan Druart from comment #35) > Created attachment 41353 [details] [review] [review] > Bug 13618: Fix error 'Not a GLOB reference' > > The interpolation of a variable on including a file caused an unexpected > error: > Template process failed: undef error - Not a GLOB reference at > /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. > > The easier fix is to replace it with a SWITCH. Did you considered adding .raw in the end of the Koha.Preference( 'AddressFormat' ) ? It would be more generalized code and it works.
Created attachment 42093 [details] [review] Bug 13618: Specific for pagination_bar
Created attachment 42094 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42095 [details] [review] Bug 13618: Specific for ColumnsSettings Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
Created attachment 42096 [details] [review] Bug 13618: Specific for pagination_bar Last time for some reason I didn't sign-off this even though I supposed to. Patch looks good. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> variables - intra
(In reply to Jonathan Druart from comment #28) > Created attachment 41347 [details] [review] [review] > Bug 13618: Remove html filters at the OPAC > > This patch removes the html filters at the OPAC, if necessary. > > Generated with: > perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc @@ -85,7 +85,7 @@ [% END %] ). [% END %] - <a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url %]&count=[% countrss |html %]&sort_by=acqdate_dsc&forma~ + <a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2~ [% END # / IF total %] </p> [% END # / IF searchdesc %] Shouldn't that one also be removed?
(In reply to Joonas Kylmälä from comment #68) > (In reply to Jonathan Druart from comment #28) > > Created attachment 41347 [details] [review] [review] [review] > > Bug 13618: Remove html filters at the OPAC > > > > This patch removes the html filters at the OPAC, if necessary. > > > > Generated with: > > perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc > > @@ -85,7 +85,7 @@ > [% END %] > ). > [% END %] > - <a href="[% OPACBaseURL > %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url > %]&count=[% countrss |html %]&sort_by=acqdate_dsc&forma~ > + <a href="[% OPACBaseURL > %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url > %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2~ > [% END # / IF total %] > </p> > [% END # / IF searchdesc %] > > Shouldn't that one also be removed? Also in this patch there was two more of these so they should be maybe removed also if this one is. I used regex ^\+.+\|html to find these
Created attachment 42097 [details] [review] Bug 13618: Remove html filters at the intranet Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>
(In reply to Jonathan Druart from comment #37) > Created attachment 41354 [details] [review] [review] > Bug 13618: Specific for IntranetUser* and OPACUser* prefs --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/sco-main.tt @@ -8,7 +8,7 @@ <link rel="stylesheet" type="text/css" href="[% interface %]/[% theme %]/lib/bootstrap/css/bootstrap.min.css" /> <link rel="stylesheet" type="text/css" href="[% interface %]/[% theme %]/lib/jquery/jquery-ui.css" /> <link rel="stylesheet" type="text/css" href="[% interface %]/[% theme %]/css/sco.css" /> -[% IF ( OPACUserCSS ) %]<style type="text/css">[% OPACUserCSS %]</style>[% END %] +[% IF ( OPACUserCSS ) %]<style type="text/css">[% OPACUserCSS.raw %]</style>[% END %] [% IF ( SCOUserCSS ) %]<style type="text/css">[% SCOUserCSS %]</style>[% END %] Why not have also raw SCOUserCSS if we let the user have OPACUserCSS as raw?
The front page in Staff side doesn't render html under News->What's Next.
(In reply to Joonas Kylmälä from comment #68) > (In reply to Jonathan Druart from comment #28) > > Created attachment 41347 [details] [review] [review] [review] > > Bug 13618: Remove html filters at the OPAC > > > > This patch removes the html filters at the OPAC, if necessary. > > > > Generated with: > > perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc > > @@ -85,7 +85,7 @@ > [% END %] > ). > [% END %] > - <a href="[% OPACBaseURL > %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url > %]&count=[% countrss |html %]&sort_by=acqdate_dsc&forma~ > + <a href="[% OPACBaseURL > %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url > %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2~ > [% END # / IF total %] > </p> > [% END # / IF searchdesc %] > > Shouldn't that one also be removed? I don't think so, I think I have tested them. (In reply to Joonas Kylmälä from comment #71) > Why not have also raw SCOUserCSS if we let the user have OPACUserCSS as raw? Done in another patch. (In reply to Joonas Kylmälä from comment #72) > The front page in Staff side doesn't render html under News->What's Next. New patch pushed to the remote branch.
Jonathan, The commits in GitHub branch look good.
(In reply to Joonas Kylmälä from comment #74) > Jonathan, The commits in GitHub branch look good. I have added you signed-off-by line on the 2 last patch. Feel free to mark the bug report as Signed off if you feel confident enough.
Remote branch rebased.
(In reply to Jonathan Druart from comment #76) > Remote branch rebased. Needs a new rebase, there are some conflicts
Hi Jonathan, all patches apply, last patch signeed. I have a followup to remove some tabs/spaces Do I have to upload all here? Also found some "| html | html_line_break " and "| html | url " Don't know if they need to be removed git grep "|html " koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/members_results.tt:20: "[% data.borrowernotes.replace('\\\\' , '\\\\') |html |html_line_break |collapse %]", koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/tables/members_results.tt:29: "[% data.borrowernotes.replace('\\\\' , '\\\\') |html |html_line_break |collapse %]", koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt:44: No results found for that in [% LibraryName %] catalog. <a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi | html | url %]&format=rss2" class="rsssearchlink noprint"><img src="[% interface %]/[% theme %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" border="0" class="rsssearchicon"/></a> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt:88: <a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi | url %][% limit_cgi |html | url %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink noprint"><img src="[% interface %]/[% theme %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-search-history.tt:71: <td><a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi |html |url %][% limit_cgi |html | url %]&[% s.query_cgi %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink"><img src="[% interface %]/[% theme %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi %]">[% s.query_desc %]</a></td> git grep "| html " koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt:111: <td><a href="[% OPACBaseURL %]/cgi-bin/koha/opac-search.pl?[% query_cgi |html |url %][% limit_cgi |html | url %]&[% s.query_cgi %]&count=[% countrss %]&sort_by=acqdate_dsc&format=rss2" class="rsssearchlink"><img src="[% interface %]/[% theme %]/images/feed-icon-16x16.png" alt="Subscribe to this search" title="Subscribe to this search" class="rsssearchicon"/></a> <a href="/cgi-bin/koha/opac-search.pl?[% s.query_cgi %]">[% s.query_desc %]</a></td>
(In reply to Bernardo Gonzalez Kriegel from comment #79) > Hi Jonathan, > all patches apply, last patch signeed. > I have a followup to remove some tabs/spaces Where? > Do I have to upload all here? No, I will add your signoffs on my branch. > Also found some "| html | html_line_break " and "| html | url " > Don't know if they need to be removed I don't think they should.
Created attachment 45141 [details] [review] Bug 13618: followup to remove tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This followup on top of remote branch Only remove tabs and trailing spaces to make koha-qa pass
(In reply to Jonathan Druart from comment #80) > (In reply to Bernardo Gonzalez Kriegel from comment #79) > > Hi Jonathan, > > all patches apply, last patch signeed. > > I have a followup to remove some tabs/spaces > > Where? > I attached a patch with fixes needed by koha-qa
(In reply to Bernardo Gonzalez Kriegel from comment #82) > (In reply to Jonathan Druart from comment #80) > > (In reply to Bernardo Gonzalez Kriegel from comment #79) > > > Hi Jonathan, > > > all patches apply, last patch signeed. > > > I have a followup to remove some tabs/spaces > > > > Where? > > > I attached a patch with fixes needed by koha-qa Thanks, added to the remote branch. Feel free to switch the status to Signed Off :)
Checked opac and staff pages (not all), no problems found
Mmm, some problems editing records and items There is a strange display on edit/add On items it can be fixed with something like --- a/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt @@ -225,9 +225,9 @@ function confirm_deletion() { [% FOREACH ite IN item %] <li><div class="subfield_line" style="[% ite.visibility %]" id="subfield[% ite.tag %][% ite.subfield %][% ite [% IF ( ite.mandatory ) %] - <label class="required">[% ite.subfield %] - [% ite.marc_lib %]</label> + <label class="required">[% ite.subfield %] - [% ite.marc_lib.raw %]</label> [% ELSE %] - <label>[% ite.subfield %] - [% ite.marc_lib %]</label> + <label>[% ite.subfield %] - [% ite.marc_lib.raw %]</label> [% END %] [% SET mv = ite.marc_value %] @@ -258,7 +258,7 @@ function confirm_deletion() { [% ELSE %] <a href="#" id="buttonDot_[%- mv.id -%]" class="[%- mv.class -%]" title="Tag editor">...</a> [% END %] - [%- mv.javascript -%] + [%- mv.javascript.raw -%] [% ELSIF ( mv.type == 'text' ) %] <input type="text" id="[%- mv.id -%]" name="field_value" class="input_marceditor" size="50" maxlength="[ [% ELSIF ( mv.type == 'textarea' ) %] But there are some javascript errors in the console. And we need a fix for record editor.
Good catch Bernardo. Fixed. commit de101ae8b8e04eeb04abe83cbb69cc02bfa87eff Bug 13618: Fix for edit biblios and items On editing biblios or items, the marc_lib, marc_value and javascript values are often populated with html code which needs to be displayed raw.
Please guys, we need a quick push of this one.
Wanted to test, got: Applying: Bug 13618: Remove html filters at the OPAC fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-authoritiessearchresultlist.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Please use the remote branch.
(In reply to Jonathan Druart from comment #89) > Please use the remote branch. Hi, there is a easy conflict on koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc :) Record and item editor now look good, but on item editor there is a 'Uncought SyntaxError: Unxepected token &', seems to come from var columns_settings = [{"columnname":"barcode"}, {"columnname":"dateaccessioned"}, ... {"columnname":"stocknumber"}] No errors on record editor or plugins, there is a warning on MARC 006/008 "Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check http://xhr.spec.whatwg.org/."
(In reply to Bernardo Gonzalez Kriegel from comment #90) > (In reply to Jonathan Druart from comment #89) > > Please use the remote branch. > > Hi, there is a easy conflict on > koha-tmpl/intranet-tmpl/prog/en/includes/doc-head-close.inc :) Rebased. > Record and item editor now look good, > but on item editor there is a 'Uncought SyntaxError: Unxepected token &', > seems to come from > > var columns_settings = [{"columnname":"barcode"}, > {"columnname":"dateaccessioned"}, > ... > {"columnname":"stocknumber"}] Introduced by a new patch, fixed in a new commit "Bug 13618: (follow-up) Specific for ColumnsSettings" > No errors on record editor or plugins, there is a warning on MARC 006/008 > "Synchronous XMLHttpRequest on the main thread is deprecated because of its > detrimental effects to the end user's experience. For more help, check > http://xhr.spec.whatwg.org/." Could you confirm you don't have it on master?
(In reply to Jonathan Druart from comment #91) > > Rebased. Ok! > > Introduced by a new patch, fixed in a new commit "Bug 13618: (follow-up) > Specific for ColumnsSettings" No more error > Could you confirm you don't have it on master? It's on master, no a problem of this patchset. No koha-qa errors. Changed to Signed-Off
(In reply to Bernardo Gonzalez Kriegel from comment #92) > (In reply to Jonathan Druart from comment #91) > Changed to Signed-Off Thanks Bernardo! I have added your signed-off-by line to the last 2 patches.
Created attachment 45784 [details] Screenshot opac-shelves.pl on /cgi-bin/koha/opac-shelves.pl the page returned's styling/layout goes funny with this patch set. Please see attached screenshot. Liz
Created attachment 45785 [details] List pages in OPAC also broken when the list contains items
Actually, please look over the lists functions in the OPAC generally, as they really seem worse for the wear after this patch.
Created attachment 45788 [details] [review] Bug 13618: (follow-up) add missing lines for opac-shelves Proposed patch to fix opac-shelves
(In reply to Liz Rea from comment #96) > Actually, please look over the lists functions in the OPAC generally, as > they really seem worse for the wear after this patch. Could you test with last patch on top?
(In reply to Bernardo Gonzalez Kriegel from comment #98) > (In reply to Liz Rea from comment #96) > > Actually, please look over the lists functions in the OPAC generally, as > > they really seem worse for the wear after this patch. > > Could you test with last patch on top? Thanks Bernardo, it fixes the issue. I have signed off the patch and pushed it to the branch. It comes from a merge conflict (on Bug 13618: Remove html filters at the OPAC) which I have not fixed correctly in one of the many rebases.
Branches rebased against master.
Branch rebased against master.
Hi Jonathan, as promised, started testing this. I checked out your branch, without rebasing it to current master. I had to install 2 dependencies via cpanm: sudo cpanm Template::Stash::HTML::Entities sudo cpanm Template::Stash::AutoEscaping We need to make sure those can be packaged! Some glitches I found: Prio 1: - HTML printer slips are broken To test: Patron account > print quick slip Others: - Patron account in staff > notices tab > HTML notices To test: use an HTML version of a check-in or check-out notice - System preferences containing HTML tags display them To test: search for 'note' in system preferences - Staff > detail page > MARC view > Items tab > displays in URL column To test: Add a 952$u to one of multiple items Some I am not totally sure about, but there is a change in behaviour to before: - HTML tags in restriction/debarment comments - HTML tags in messages in the patron account ('new message' link) Stopping my testing for now.
Not seeing any immediate bar to attempting to package the new deps, but I should emphasize that new dependencies means that this is not backportable as far as the packages are concerned. To emphasize this point, I've recategorized this bug as an enhancement, as it's more about implementing a global solution rather than fixing a bunch of individual XSS issues.
(In reply to Katrin Fischer from comment #102) Everything has been fixed in the following new commits: commit 13faef4d28df0a70c2c89ab4317ebc2882fc3704 Bug 13618: Fix for system preference description If a syspref description contains html tag, do not display them commit 2879a9976549f84c9520d5778a45d8348845d7b4 Bug 13618: Do not display and html tags in item fields content Note that there might be other occurrences to fix! commit d8547698a47e83cdb7d321391cadddd4f26e5f85 Bug 13618: Do not display html tags in patron's notices commit 58e62b53eda1110003d6c0b0cebc983c62bf2f46 Bug 13618: Fix for debarredcomment and patron messages At the OPAC and intranet. commit 0aefdddb394db1fab7287970ab0b80f3a92479e8 Bug 13618: (follow-up) Specific for other prefs follow-up for SlipCSS Remote branch updated.
Katrin, feel free to ask to new signoffs on these patches if you think it's needed.
Hi Jonathan, I (re)testd some more: NOW OK: - Patron account in staff > notices tab > HTML notices - System preferences containing HTML tags display them To test: search for 'note' in system preferences - Staff > detail page > MARC view > Items tab > displays in URL column To test: Add a 952$u to one of multiple items - HTML tags in messages in the patron account ('new message' link) NOT OK: - HTML printer slips are broken To test: Patron account > print quick slip This shows still the same problem for and I don't see a patch with a matching description on the branch? - HTML tags in restriction/debarment comments Works partially - the restriction message on top of the patron account is ok. The entries in the little restrictions table inside the tab still appear broken - on the details and checkout tabs and also when editing a patron. Other test results: - OPAC is looking good, nothing found - Staff is looking good as well - no new findings. :)
One more thing - I had to install 2 dependencies to get it to work. - Should both go into the PerlDependencies.pm file?
Last 2 patches updated on the remote branch.
(In reply to Katrin Fischer from comment #102) > I had to install 2 dependencies via cpanm: > sudo cpanm Template::Stash::HTML::Entities > sudo cpanm Template::Stash::AutoEscaping Are you sure about that? I have removed Template::Stash::HTML::Entities and everything looks fine.
But comment#3 told me to install it! Maybe it's not needed then :) I can retest this as well.
Hi Jonathan, working on this again - can you please update your branch to master once more?
Retesting the issues found last time: - HTML printer slips work perfectly now - HTML tags in restriction/debarment comments show ... and while you are on it, could you please add my sign off lines as well? :) I am going to obsolete the patches here for now, to avoid confusion.
Works partially - the restriction message on top of the patron account is ok. The entries in the little restrictions table inside the tab still appear broken - on the details and checkout tabs and also when editing a patron.variables - intra
Created attachment 46767 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.) Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46768 [details] [review] Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46769 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46770 [details] [review] Bug 13618: Remove html filters at the intranet Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46771 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46772 [details] [review] Bug 13618: Specific for pagination_bar Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46773 [details] [review] Bug 13618: Specific for the ISBD view Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46774 [details] [review] Bug 13618: Fix error 'Not a GLOB reference' The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. The easier fix is to replace it with a SWITCH. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46775 [details] [review] Bug 13618: Specific for IntranetUser* and OPACUser* prefs Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46776 [details] [review] Bug 13618: Specific for ColumnsSettings Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46777 [details] [review] Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46778 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46779 [details] [review] Bug 13618: Specific for Salutation on editing a patron Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46780 [details] [review] Bug 13618: Specific for XSLTBloc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46781 [details] [review] Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46782 [details] [review] Bug 13618: Fix for news Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46783 [details] [review] Bug 13618: Fix last occurrences recently introduced to master Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46784 [details] [review] Bug 13618: followup to remove tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This followup on top of remote branch Only remove tabs and trailing spaces to make koha-qa pass Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46785 [details] [review] Bug 13618: Fix for edit biblios and items On editing biblios or items, the marc_lib, marc_value and javascript values are often populated with html code which needs to be displayed raw. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46786 [details] [review] Bug 13618: (follow-up) Specific for ColumnsSettings Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46787 [details] [review] Bug 13618: (follow-up) add missing lines for opac-shelves Proposed patch to fix opac-shelves Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46788 [details] [review] Bug 13618: Remove html filters for newly pushed code Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46789 [details] [review] Bug 13618: Fix for system preference description If a syspref description contains html tag, do not display them Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46790 [details] [review] Bug 13618: Do not display and html tags in item fields content Note that there might be other occurrences to fix! Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46791 [details] [review] Bug 13618: Do not display html tags in patron's notices Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46792 [details] [review] Bug 13618: Fix for debarredcomment and patron messages At the OPAC and intranet. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46793 [details] [review] Bug 13618: (follow-up) Specific for other prefs follow-up for SlipCSS and printslip Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 46794 [details] [review] Bug 13618: Specific for branches.opac_info
(In reply to Katrin Fischer from comment #112) > Retesting the issues found last time: > - HTML printer slips work perfectly now > - HTML tags in restriction/debarment comments show > > ... and while you are on it, could you please add my sign off lines as well? > :) > > I am going to obsolete the patches here for now, to avoid confusion. I have added your signed-off-by line, attached the patches from the remote branch on this bug report and attached a new patch (found a bug when rebasing).
Created attachment 46795 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.) Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46796 [details] [review] Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46797 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46798 [details] [review] Bug 13618: Remove html filters at the intranet Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46799 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46800 [details] [review] Bug 13618: Specific for pagination_bar Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46801 [details] [review] Bug 13618: Specific for the ISBD view Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46802 [details] [review] Bug 13618: Fix error 'Not a GLOB reference' The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. The easier fix is to replace it with a SWITCH. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46803 [details] [review] Bug 13618: Specific for IntranetUser* and OPACUser* prefs Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46804 [details] [review] Bug 13618: Specific for ColumnsSettings Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46805 [details] [review] Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46806 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46807 [details] [review] Bug 13618: Specific for Salutation on editing a patron Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46808 [details] [review] Bug 13618: Specific for XSLTBloc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46809 [details] [review] Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46810 [details] [review] Bug 13618: Fix for news Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46811 [details] [review] Bug 13618: Fix last occurrences recently introduced to master Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46812 [details] [review] Bug 13618: followup to remove tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This followup on top of remote branch Only remove tabs and trailing spaces to make koha-qa pass Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46813 [details] [review] Bug 13618: Fix for edit biblios and items On editing biblios or items, the marc_lib, marc_value and javascript values are often populated with html code which needs to be displayed raw. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46814 [details] [review] Bug 13618: (follow-up) Specific for ColumnsSettings Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46815 [details] [review] Bug 13618: (follow-up) add missing lines for opac-shelves Proposed patch to fix opac-shelves Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46816 [details] [review] Bug 13618: Remove html filters for newly pushed code Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46817 [details] [review] Bug 13618: Fix for system preference description If a syspref description contains html tag, do not display them Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46818 [details] [review] Bug 13618: Do not display and html tags in item fields content Note that there might be other occurrences to fix! Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46819 [details] [review] Bug 13618: Do not display html tags in patron's notices Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46820 [details] [review] Bug 13618: Fix for debarredcomment and patron messages At the OPAC and intranet. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46821 [details] [review] Bug 13618: (follow-up) Specific for other prefs follow-up for SlipCSS and printslip Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 46822 [details] [review] Bug 13618: Specific for branches.opac_info
Jonathan, can you please rebase these patches? Thanks!
Created attachment 47425 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.) Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47426 [details] [review] Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47427 [details] [review] Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47428 [details] [review] Bug 13618: Remove html filters at the intranet Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47429 [details] [review] Bug 13618: Specific places where we don't need to escape variables - intra Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47430 [details] [review] Bug 13618: Specific for pagination_bar Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47431 [details] [review] Bug 13618: Specific for the ISBD view Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47432 [details] [review] Bug 13618: Fix error 'Not a GLOB reference' The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. The easier fix is to replace it with a SWITCH. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47433 [details] [review] Bug 13618: Specific for IntranetUser* and OPACUser* prefs Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47434 [details] [review] Bug 13618: Specific for ColumnsSettings Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47435 [details] [review] Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47436 [details] [review] Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47437 [details] [review] Bug 13618: Specific for Salutation on editing a patron Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47438 [details] [review] Bug 13618: Specific for XSLTBloc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47439 [details] [review] Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47440 [details] [review] Bug 13618: Fix for news Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47441 [details] [review] Bug 13618: Fix last occurrences recently introduced to master Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47442 [details] [review] Bug 13618: followup to remove tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This followup on top of remote branch Only remove tabs and trailing spaces to make koha-qa pass Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47443 [details] [review] Bug 13618: Fix for edit biblios and items On editing biblios or items, the marc_lib, marc_value and javascript values are often populated with html code which needs to be displayed raw. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47444 [details] [review] Bug 13618: (follow-up) Specific for ColumnsSettings Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47445 [details] [review] Bug 13618: (follow-up) add missing lines for opac-shelves Proposed patch to fix opac-shelves Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47446 [details] [review] Bug 13618: Remove html filters for newly pushed code Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47447 [details] [review] Bug 13618: Fix for system preference description If a syspref description contains html tag, do not display them Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47448 [details] [review] Bug 13618: Do not display and html tags in item fields content Note that there might be other occurrences to fix! Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47449 [details] [review] Bug 13618: Do not display html tags in patron's notices Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47450 [details] [review] Bug 13618: Fix for debarredcomment and patron messages At the OPAC and intranet. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47451 [details] [review] Bug 13618: (follow-up) Specific for other prefs follow-up for SlipCSS and printslip Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 47452 [details] [review] Bug 13618: Specific for branches.opac_info
@RM: please don't squash these patches, it will be helpful to have the history if a regression if found later.
(In reply to Jonathan Druart from comment #200) > @RM: please don't squash these patches, it will be helpful to have the > history if a regression if found later. Will not squash :)
Pushed to Master - Should be in the May 2016 Release. Thanks! (we'll probably have some more spots to work on this). Great job though!
Great work! Detail for patch: Use Template::Stash::AutoEscaping to use the html filter Note that this still needs attention in terms of (Debian) packaging. At first glance this module does not seem to be available in a Debian package. Obviously, new perl dependencies should preferably be available already.
New test added to the QA script: commit a9ddabb936a0a1539d01992908b3e6484b6d6466 The html filter is not needed anymore (see bug 13618)
(In reply to Marcel de Rooy from comment #203) > Note that this still needs attention in terms of (Debian) packaging. > At first glance this module does not seem to be available in a Debian > package. I started packaging this module. ITP bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813574 Git repo: https://anonscm.debian.org/cgit/pkg-perl/packages/libtemplate-stash-autoescaping-perl.git/
(In reply to Julian Maurice from comment #205) > I started packaging this module. > ITP bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=813574 > Git repo: > https://anonscm.debian.org/cgit/pkg-perl/packages/libtemplate-stash- > autoescaping-perl.git/ Great! Thx
Created attachment 47631 [details] libtemplate-stash-autoescaping-perl_0.0303-1_all.deb I built a package in Jessie, maybe it is useful for others too. It's attached.
Package accepted into Debian unstable: https://packages.debian.org/sid/libtemplate-stash-autoescaping-perl
Thx Julian!
Reverted from master. Making Status as Blocked. Jonathan please choose the status you'd prefer for this bug.
See bug 15715 for the cause of the revert.
The approach here is correct in my opinion.. we should treat everything as unsafe and thus escape it by default unless we've manually checked it and marked as safe. Yes, this will impact performance at first and highlight some especially nasty areas of koha. We should use those highlights as a hitlist of areas to concentrate on a) checking security and marking as safe when possible and b) refactoring templates to more sparsely use variables when possible.
Created attachment 59983 [details] [review] Bug 13618: Use Template::Stash::AutoEscaping to use the html filter Test plan: 0/ sudo cpanm Template::Stash::AutoEscaping 1/ Verify don't reproduce the XSS issue described on bug 13609 and other xss related bugs. 2/ Try to find some encoding issues (detail page, search results, facets, etc.) Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Remove html filters at the OPAC This patch removes the html filters at the OPAC, if necessary. Generated with: perl -p -i -e 's/\ ?\|\ ?html(\ ?)%/\1%/g' **/*.tt **/*.inc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific places where we don't need to escape variables There is no need to escape the html generated by the XSLT. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Remove html filters at the intranet Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific places where we don't need to escape variables - intra Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for pagination_bar Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for the ISBD view Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Fix error 'Not a GLOB reference' The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. The easier fix is to replace it with a SWITCH. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for IntranetUser* and OPACUser* prefs Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for ColumnsSettings Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618 - memberentrygen.tt errors Not a GLOB reference Like Jonathan said: The interpolation of a variable on including a file caused an unexpected error: Template process failed: undef error - Not a GLOB reference at /usr/lib/i386-linux-gnu/perl5/5.20/Template/Provider.pm line 619. Replaced it with a SWITCH, like the other patch for this similar error. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for other prefs opacmainuserblock opacnav opacnavright opaccredits opacheader opaccustomsearch opacmysummaryhtml opacmysummarynote opacnavbottom opacnoresultsfound opacresultssidebar opacsearchfortitlein restrictedpagecontent PatronSelfRegistrationAdditionalInstructions intranetmainuserblock intranetnav intranetslipprinterjs OpacSuppressionMessage SCOUserCSS SCOUserJS SelfCheckHelpMessage NoLoginInstructions Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for Salutation on editing a patron Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Specific for XSLTBloc Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Fix escape on sending baskets or shelves by email Test plan: Send baskets and shelves by email. With or without this patch, you should not see any changes. Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Fix for news Signed-off-by: Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: Fix last occurrences recently introduced to master Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: followup to remove tabs Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This followup on top of remote branch Only remove tabs and trailing spaces to make koha-qa pass Bug 13618: Fix for edit biblios and items On editing biblios or items, the marc_lib, marc_value and javascript values are often populated with html code which needs to be displayed raw. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: (follow-up) Specific for ColumnsSettings Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Bug 13618: (follow-up) add missing lines for opac-shelves Proposed patch to fix opac-shelves Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Bug 13618: Remove html filters for newly pushed code Bug 13618: Fix for system preference description If a syspref description contains html tag, do not display them Bug 13618: Do not display and html tags in item fields content Note that there might be other occurrences to fix! Bug 13618: Do not display html tags in patron's notices Bug 13618: Fix for debarredcomment and patron messages At the OPAC and intranet. Bug 13618: (follow-up) Specific for other prefs follow-up for SlipCSS and printslip
Created attachment 59984 [details] [review] Bug 13618: Use a simplified version of Template::Stash::AutoEscaping Koha::Template::Escape is a simple version of Template::Stash::AutoEscaping It seems that removing and cleanly some pieces of Template::Stash::AutoEscaping will bring us a lot of performance boost.
Created attachment 59987 [details] [review] Bug 13618: Specific for branches.opac_info Forgot this one when I squashed the others.
Fun is coming back guys \o/ It seems that I get rid of the performance problem, see the second patch.
Patches have been rebased, adjusted for master and push to https://gitlab.com/joubu/Koha/tree/bug_13618 They do not deal with performance issues but are ready to be tested (to catch missing .raw)
(In reply to Jonathan Druart from comment #217) > Patches have been rebased, adjusted for master and push to > https://gitlab.com/joubu/Koha/tree/bug_13618 > > They do not deal with performance issues but are ready to be tested (to > catch missing .raw) No need to test if we do not have a solution on bug 20975.
I am back! Next version has been pushed to the remote branch - https://gitlab.com/joubu/Koha/commits/bug_13618 Here is commit message of the main patch: As we did not fix the performance issue when autofiltering the variables (see bug 20975), the only solution we have is to add the filters explicitely. This patch has been autogenerated (using add_html_filters.pl, see next pathces) and add the html filter to all the variables displayed in the template. Exceptions are made (using the new 'raw' TT filter) to the variable we already listed in the previous versions of this patch. To test: - Use t/db_dependent/Koha/Patrons.t to populate your DB with autogenerated data which contain <script> tags - Remove them from borrower_debarments.comments (there are allowed here) update borrower_debarments set comment="html tags possible here"; - From the interface hit page and try to catch alert box. If you find one it means you find a possible XSS. To know where it comes from: * note the exact URL where you found it * note the alert box content * Dump your DB and search for the string in the dump to identify its location (for instance table.field) Next: * Ideally we would like to use the raw filter when it is not necessary to HTML escape the variables (in big loop for instance) * Provide a QA script to catch missing filters (we want html, uri, url or raw, certainly others that I am forgetting now) * Replace the html filters with uri when needed (!) Please test!
Looks to be going along the right lines to me, I like the approach and can see it leading to a much better base to start with. Maintaining it going forward is the next challenge.. hopefully some carefully placed git hooks could help there, or adding some logic to the qa script to try and catch them? I've also often wondered about having a community arachni server (or some other automated penetration testing system) setup and hitting a clone of master regularly... think I've brought that idea up before but didn't have the time to pursue it.
I did what I hope was a fairly thorough test of the staff client and found these issues: - IntranetCirculationHomeHTML displays HTML tags as text - Patron title include showing HTML: <span class="patron-title">Mr</span> - Patron details -> Holds tab: Alerts data from the branches table - Search results page layout is broken. Looks like page-numbers.inc has a section missing. - Crazy encoding of action buttons on Lists page - Incorrectly escaped HTML in Notices & slips list - Label batch list title encoding wrong - Spine label print shows HTML - Administration -> Libraries: Alerts data from the branches table - Administration -> Item types: Alerts data from the items table - Item searching broken: "Unsupported format html at /home/vagrant/kohaclone/catalogue/itemsearch.pl line 42."
(In reply to Owen Leonard from comment #221) > I did what I hope was a fairly thorough test of the staff client and found > these issues: > > - IntranetCirculationHomeHTML displays HTML tags as text Done, specific patch for this pref. > - Patron title include showing HTML: <span > class="patron-title">Mr</span> Done, see specific patch. > - Patron details -> Holds tab: Alerts data from the branches table Done, that was tricky and a part I forgot, we need to escape data using JS, see String.prototype.escapeHtml > - Search results page layout is broken. Looks like page-numbers.inc has a > section missing. Ooops, wrong merge conflict resolution. > - Crazy encoding of action buttons on Lists page > - Incorrectly escaped HTML in Notices & slips list Both fixed now. > - Label batch list title encoding wrong > - Spine label print shows HTML Fixed but follow-ups needed (TODO LATER) > - Administration -> Libraries: Alerts data from the branches table It comes from opac_info, which can contain html characters. See admin/branches.tt: library.opac_info is not escaped (" | $raw") > - Administration -> Item types: Alerts data from the items table Same as before for itemtype.checkinmsg. I have added a patch for the missing $raw filter to make it explicit. > - Item searching broken: "Unsupported format html at > /home/vagrant/kohaclone/catalogue/itemsearch.pl line 42." Done, that was a hard one!
Found two more issues: - The branches table is alerting something on ILL requests -> New request - IntranetUserJS incorrectly outputs encoded HTML entities
(In reply to Owen Leonard from comment #224) > Found two more issues: > > - The branches table is alerting something on ILL requests -> New request This comes from ILL backends (not in Koha). > - IntranetUserJS incorrectly outputs encoded HTML entities Fixed in a new patch.
I have tested this to the best of my ability and found it to be working! **************** ** Signed off ** ****************
(In reply to Owen Leonard from comment #226) > I have tested this to the best of my ability and found it to be working! > > **************** > ** Signed off ** > **************** Signed-off-by lines added to the remote branch, thanks for testing! I also have rebased the remote branch against master.
I've spent a few hours with this one this evening... The QA script raises a few warnings, but I think they're false positives or pre-existing issues in all cases at this point. I'm sure there will be issues with a patchset of this magnitude, but I am confident we will correct them before release and that they would not be caught at this point by delaying any further. ********** Passing QA **********
Created attachment 77804 [details] [review] Bug 13618: [DO NOT PUSH] have fun In order to generate quickly a lot of relevant data I have modified a bit TestBuilder->_gen_text to insert <script> tags in DB. The transaction have been removed from t/db_dependent/Koha/Patrons.t To use it, use a clean data (sample data only) the run t/db_dependent/Koha/Patrons.t (only once, it will fail it you run it more) Search for patrons, without this patch you will get a looot of alert boxes. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 77805 [details] [review] Bug 13618: [DO NOT PUSH] have fun In order to generate quickly a lot of relevant data I have modified a bit TestBuilder->_gen_text to insert <script> tags in DB. The transaction have been removed from t/db_dependent/Koha/Patrons.t To use it, use a clean data (sample data only) the run t/db_dependent/Koha/Patrons.t (only once, it will fail it you run it more) Search for patrons, without this patch you will get a looot of alert boxes. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 77806 [details] [review] Bug 13618: [DO NOT PUSH] have fun In order to generate quickly a lot of relevant data I have modified a bit TestBuilder->_gen_text to insert <script> tags in DB. The transaction have been removed from t/db_dependent/Koha/Patrons.t To use it, use a clean data (sample data only) the run t/db_dependent/Koha/Patrons.t (only once, it will fail it you run it more) Search for patrons, without this patch you will get a looot of alert boxes. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I was going to upload the patches for you Nick, but it turns out some of them are too large for bugzilla so Jonathan is adding my signoff lines to his tracking branch on gitlab ;) Note, bug 15717 is rolled into this branch patchset.. it all checks out too ;)
Created attachment 77807 [details] [review] Bug 13618: [DO NOT PUSH] have fun In order to generate quickly a lot of relevant data I have modified a bit TestBuilder->_gen_text to insert <script> tags in DB. The transaction have been removed from t/db_dependent/Koha/Patrons.t To use it, use a clean data (sample data only) the run t/db_dependent/Koha/Patrons.t (only once, it will fail it you run it more) Search for patrons, without this patch you will get a looot of alert boxes. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 77808 [details] [review] Bug 13618: [DO NOT PUSH] Add script to add the html filters Usage: perl add_html_filters.pl **/*.inc **/*.tt It also takes --test for debugging Then: git grep Asset|grep -v USE | grep -v '| $raw' => We should deal with them on a separate commit Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The two patches used to build this patchset and test it have been attached to this bug report. Others are not as the first one is too big (413 Request Entity Too Large). Martin's Signed-off-by lines have been added to my remove branch and the "DO NOT PUSH" patches removed.
Created attachment 77809 [details] patches_to_be_pushed.tar.gz For reference.
Awesome work all, special thanks to Jonathan for working on this one for so long and to everyone who helped along the way. Pushed to master for 18.11
Created attachment 77997 [details] [review] Bug 13618: Remove filter when assigning array
Created attachment 77998 [details] [review] Bug 13618: Fix xt/tt_valid.t We are going to say it quietly but this test does not catch the problem when there are no spaces....shhhh Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #239) > Created attachment 77998 [details] [review] [review] > Bug 13618: Fix xt/tt_valid.t > > We are going to say it quietly but this test does not catch the problem > when there are no spaces....shhhh > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Followup pushed to master
Coding guidelines: https://wiki.koha-community.org/wiki/Coding_Guidelines#HTML9:_filter_all_the_variables
Created attachment 78158 [details] [review] Bug 13618: (follow-up) Manually replace missing .raw Must be |$raw, not |raw Error: Template process failed: undef error - raw: filter not found at /home/vagrant/kohaclone/C4/Templates.pm line 122 To recreate: Add a new restriction and visit circulation.pl?borrowernumber=42
(In reply to Jonathan Druart from comment #242) > Created attachment 78158 [details] [review] [review] > Bug 13618: (follow-up) Manually replace missing .raw > > Must be |$raw, not |raw > > Error: > Template process failed: undef error - raw: filter not found at > /home/vagrant/kohaclone/C4/Templates.pm line 122 > > To recreate: > Add a new restriction and visit circulation.pl?borrowernumber=42 Followup pushed to master
Looks like escaping was added also to places where it doesn't belong. The examples I stumbled on were just setting a variable: https://github.com/Koha-Community/Koha/blob/master/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt#L328 and https://github.com/Koha-Community/Koha/blob/master/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt#L330
Created attachment 78274 [details] [review] Bug 13618: (follow-up) Add html filters to all the variables
Thanks Ere for reporting it!
(In reply to Jonathan Druart from comment #245) > Created attachment 78274 [details] [review] [review] > Bug 13618: (follow-up) Add html filters to all the variables Followup pushed to master
Created attachment 78415 [details] [review] Bug 13618: Do not use html filters with KohaSpan To recreate the issue: Go to Home › Administration › Library groups Create a new group "Group <span class='name'>xxx</span> created." Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #248) > Created attachment 78415 [details] [review] [review] > Bug 13618: Do not use html filters with KohaSpan > > To recreate the issue: > Go to Home › Administration › Library groups > Create a new group > "Group <span class='name'>xxx</span> created." > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Followup pushed to master
Lesson pasted from bug 21293: All occurrences of [% var = something | html %] are error prone. If something is not a string, but an object, array, hash etc., we are in trouble. This probably needs more attention since we are passing objects to templates in more scripts.
Created attachment 79404 [details] [review] Bug 13618: (follow-up) Pass opacuser_js from plugins as raw We expect this field to contain script tags, html processing breaks them
*** Bug 20874 has been marked as a duplicate of this bug. ***
*** Bug 20873 has been marked as a duplicate of this bug. ***
*** Bug 20872 has been marked as a duplicate of this bug. ***
*** Bug 20871 has been marked as a duplicate of this bug. ***
*** Bug 20869 has been marked as a duplicate of this bug. ***
*** Bug 20870 has been marked as a duplicate of this bug. ***
(In reply to Nick Clemens from comment #251) > Created attachment 79404 [details] [review] [review] > Bug 13618: (follow-up) Pass opacuser_js from plugins as raw > > We expect this field to contain script tags, html processing breaks them Follow-up pushed to master
A new check to catch missing filters is not in the QA script, please update!
(In reply to Jonathan Druart from comment #259) > A new check to catch missing filters is not in the QA script, please update! s/not/now/
Created attachment 79818 [details] [review] Bug 13618: remove wrong html filter in 'Item search fields' admin The dropdown list contain the arrayref: ARRAY(0x...) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
*** Bug 20909 has been marked as a duplicate of this bug. ***
Created attachment 81183 [details] [review] Bug 13618: Remove html filter for LAST Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #261) >Created attachment 79818 [details] [review] [review] >Bug 13618: remove wrong html filter in 'Item search fields' admin > >The dropdown list contain the arrayref: ARRAY(0x...) > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> (In reply to Jonathan Druart from comment #263) > Created attachment 81183 [details] [review] [review] > Bug 13618: Remove html filter for LAST > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Last two followups pushed to master
http://koha.1045719.n5.nabble.com/HTML-filtering-issue-td6004477.html Fix here? Fix on another bug?
In the OPAC, when looking at the MARC view for an item, there is a table at the bottom of the page showing some holding information. Those table fields that used to be blank in 18.05 are now displaying as " " (without the quotes) in 18.11. In my particular case, the fields that are displayed this way are: * Withdrawn status * Lost status * Damaged status * Not for loan I'll attach some before-and-after screenshots. I determined that this was caused by the change to opac-MARCdetail.tt made for bug 13618, by reverting that change for line 151. That is, I changed that line from: <td>[% item.$sf_code | html %]</td> to this: <td>[% item.$sf_code %]</td> That removed the mess, but I'm guessing this is not the correct solution.
Created attachment 83479 [details] OPAC MARC view holding detail in 18.05
Created attachment 83480 [details] OPAC MARC view holding detail in 18.11
This bug has been marked resolved fixed - please file any remaining issues as new bugs and link to this one using the "depends" on the new bug.
*** Bug 22342 has been marked as a duplicate of this bug. ***
*** Bug 19121 has been marked as a duplicate of this bug. ***
*** Bug 19157 has been marked as a duplicate of this bug. ***