Created attachment 65277 [details] [review] Bug 17989: Centralize bad template check The bad template check in get_template_and_user can be removed, since this check has been added in gettemplate on bug 18010. The check moves here to a new sub in C4::Templates. And will be extended in a follow-up. Removed unused variable $path in gettemplatefile. Test plan: [1] Run t/db_dependent/Auth.t (get_template_and_user bad calls). [2] Run t/db_dependent/Templates.t (bad call to gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65278 [details] [review] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65279 [details] [review] Bug 17989: Include full path logic in _get_template_file Similar to the full path test in sub themelanguage, this patch makes a change in _get_template_file. This allows you to pass a template outside the modules directory to get_template_and_user. (Note: the sub badtemplatecheck already blocks bad paths.) Especially, this would be helpful for plugins using templates. As can be seen in Templates.pm, a change was made earlier to overwrite the filename for a plugin in sub gettemplate. This exception can now be removed. Also note the small change in Koha/Plugin/Base.pm; mbf_path is already absolute and if we pass a full path, we do not need it. This allows use of a regular Koha template or a shared template between plugins (as long as badtemplatecheck allows the path). What are the side-effects of this change? [1] We should not pass absolute paths if we mean relative ones. A follow-up patch deals with one occurrence in the codebase. No regressions for regular use. [2] Plugins can call get_template_and_user directly or go via get_template in Koha/Plugin/Base (absolute paths don't go via mbf_path). Note: replaced two single quotes in Auth.pm to show template name in test description. Test plan: [1] Open some page on OPAC or staff client to trigger a template. [2] Run t/db_dependent/Auth.t to verify not allowing some bad templates. [3] Run t/db_dependent/Templates.t to verify an absolute path. [4] Run t/db_dependent/Plugins.t to verify using templates in a plugin. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65280 [details] [review] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates *** WARNING: Verbatim paragraph in NAME section at line 28 in file C4/Templates.pm Test plan: Run podchecker on C4::Templates and verify that the warning is resolved. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 65281 [details] [review] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. [2] Script svc/members/search gets its template path from a URL parameter. When you git grep json_template, you will have an idea what values it should receive. Test plan: [1] Go intranet home page. Search for patrons in the combo box. Results? [2] Try the URL /cgi-bin/koha/svc/members/search. Should trigger a 500 error with Bad template path in the logs. [3] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66272 [details] [review] Bug 17989: Centralize bad template check The bad template check in get_template_and_user can be removed, since this check has been added in gettemplate on bug 18010. The check moves here to a new sub in C4::Templates. And will be extended in a follow-up. Removed unused variable $path in gettemplatefile. Test plan: [1] Run t/db_dependent/Auth.t (get_template_and_user bad calls). [2] Run t/db_dependent/Templates.t (bad call to gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66273 [details] [review] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66274 [details] [review] Bug 17989: Include full path logic in _get_template_file Similar to the full path test in sub themelanguage, this patch makes a change in _get_template_file. This allows you to pass a template outside the modules directory to get_template_and_user. (Note: the sub badtemplatecheck already blocks bad paths.) Especially, this would be helpful for plugins using templates. As can be seen in Templates.pm, a change was made earlier to overwrite the filename for a plugin in sub gettemplate. This exception can now be removed. Also note the small change in Koha/Plugin/Base.pm; mbf_path is already absolute and if we pass a full path, we do not need it. This allows use of a regular Koha template or a shared template between plugins (as long as badtemplatecheck allows the path). What are the side-effects of this change? [1] We should not pass absolute paths if we mean relative ones. A follow-up patch deals with one occurrence in the codebase. No regressions for regular use. [2] Plugins can call get_template_and_user directly or go via get_template in Koha/Plugin/Base (absolute paths don't go via mbf_path). Note: replaced two single quotes in Auth.pm to show template name in test description. Test plan: [1] Open some page on OPAC or staff client to trigger a template. [2] Run t/db_dependent/Auth.t to verify not allowing some bad templates. [3] Run t/db_dependent/Templates.t to verify an absolute path. [4] Run t/db_dependent/Plugins.t to verify using templates in a plugin. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66275 [details] [review] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates *** WARNING: Verbatim paragraph in NAME section at line 28 in file C4/Templates.pm Test plan: Run podchecker on C4::Templates and verify that the warning is resolved. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66276 [details] [review] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. [2] Script svc/members/search gets its template path from a URL parameter. When you git grep json_template, you will have an idea what values it should receive. Test plan: [1] Go intranet home page. Search for patrons in the combo box. Results? [2] Try the URL /cgi-bin/koha/svc/members/search. Should trigger a 500 error with Bad template path in the logs. [3] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Trivial rebase
Created attachment 66317 [details] [review] [SIGNED-OFF] Bug 17989: Centralize bad template check The bad template check in get_template_and_user can be removed, since this check has been added in gettemplate on bug 18010. The check moves here to a new sub in C4::Templates. And will be extended in a follow-up. Removed unused variable $path in gettemplatefile. Test plan: [1] Run t/db_dependent/Auth.t (get_template_and_user bad calls). [2] Run t/db_dependent/Templates.t (bad call to gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66318 [details] [review] [SIGNED-OFF] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66319 [details] [review] [SIGNED-OFF] Bug 17989: Include full path logic in _get_template_file Similar to the full path test in sub themelanguage, this patch makes a change in _get_template_file. This allows you to pass a template outside the modules directory to get_template_and_user. (Note: the sub badtemplatecheck already blocks bad paths.) Especially, this would be helpful for plugins using templates. As can be seen in Templates.pm, a change was made earlier to overwrite the filename for a plugin in sub gettemplate. This exception can now be removed. Also note the small change in Koha/Plugin/Base.pm; mbf_path is already absolute and if we pass a full path, we do not need it. This allows use of a regular Koha template or a shared template between plugins (as long as badtemplatecheck allows the path). What are the side-effects of this change? [1] We should not pass absolute paths if we mean relative ones. A follow-up patch deals with one occurrence in the codebase. No regressions for regular use. [2] Plugins can call get_template_and_user directly or go via get_template in Koha/Plugin/Base (absolute paths don't go via mbf_path). Note: replaced two single quotes in Auth.pm to show template name in test description. Test plan: [1] Open some page on OPAC or staff client to trigger a template. [2] Run t/db_dependent/Auth.t to verify not allowing some bad templates. [3] Run t/db_dependent/Templates.t to verify an absolute path. [4] Run t/db_dependent/Plugins.t to verify using templates in a plugin. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66320 [details] [review] [SIGNED-OFF] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates *** WARNING: Verbatim paragraph in NAME section at line 28 in file C4/Templates.pm Test plan: Run podchecker on C4::Templates and verify that the warning is resolved. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66321 [details] [review] [SIGNED-OFF] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. [2] Script svc/members/search gets its template path from a URL parameter. When you git grep json_template, you will have an idea what values it should receive. Test plan: [1] Go intranet home page. Search for patrons in the combo box. Results? [2] Try the URL /cgi-bin/koha/svc/members/search. Should trigger a 500 error with Bad template path in the logs. [3] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Josef Moravec from comment #16) > Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Great !
Created attachment 67514 [details] [review] Bug 17989: Centralize bad template check The bad template check in get_template_and_user can be removed, since this check has been added in gettemplate on bug 18010. The check moves here to a new sub in C4::Templates. And will be extended in a follow-up. Removed unused variable $path in gettemplatefile. Test plan: [1] Run t/db_dependent/Auth.t (get_template_and_user bad calls). [2] Run t/db_dependent/Templates.t (bad call to gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67515 [details] [review] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67516 [details] [review] Bug 17989: Include full path logic in _get_template_file Similar to the full path test in sub themelanguage, this patch makes a change in _get_template_file. This allows you to pass a template outside the modules directory to get_template_and_user. (Note: the sub badtemplatecheck already blocks bad paths.) Especially, this would be helpful for plugins using templates. As can be seen in Templates.pm, a change was made earlier to overwrite the filename for a plugin in sub gettemplate. This exception can now be removed. Also note the small change in Koha/Plugin/Base.pm; mbf_path is already absolute and if we pass a full path, we do not need it. This allows use of a regular Koha template or a shared template between plugins (as long as badtemplatecheck allows the path). What are the side-effects of this change? [1] We should not pass absolute paths if we mean relative ones. A follow-up patch deals with one occurrence in the codebase. No regressions for regular use. [2] Plugins can call get_template_and_user directly or go via get_template in Koha/Plugin/Base (absolute paths don't go via mbf_path). Note: replaced two single quotes in Auth.pm to show template name in test description. Test plan: [1] Open some page on OPAC or staff client to trigger a template. [2] Run t/db_dependent/Auth.t to verify not allowing some bad templates. [3] Run t/db_dependent/Templates.t to verify an absolute path. [4] Run t/db_dependent/Plugins.t to verify using templates in a plugin. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 67517 [details] [review] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates *** WARNING: Verbatim paragraph in NAME section at line 28 in file C4/Templates.pm Test plan: Run podchecker on C4::Templates and verify that the warning is resolved. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 67518 [details] [review] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. [2] Script svc/members/search gets its template path from a URL parameter. When you git grep json_template, you will have an idea what values it should receive. Test plan: [1] Go intranet home page. Search for patrons in the combo box. Results? [2] Try the URL /cgi-bin/koha/svc/members/search. Should trigger a 500 error with Bad template path in the logs. [3] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Simple rebase
This looks good overall, everything seems to work fine, one test breakage: vagrant@kohadevbox:kohaclone(BUGFUN)$ prove -v t/db_dependent/SuggestionEngine_ExplodedTerms.t t/db_dependent/SuggestionEngine_ExplodedTerms.t .. ok 1 - use Koha::SuggestionEngine; ok 2 - Created suggestion engine bad template path# Tests were run but no plan was declared and done_testing() was not seen. # Looks like your test exited with 255 just after 2. Dubious, test returned 255 (wstat 65280, 0xff00) All 2 subtests passed
Created attachment 68654 [details] [review] Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t SuggestionEngine_ExplodedTerms.t uses paths like /../.. or /.. in order to find a template in opac-tmpl. Sub badtemplatecheck does not like these paths. Using Cwd::abs_path to resolve the situation (rel2abs does not). Test plan: Run again t/db_dependent/SuggestionEngine_ExplodedTerms.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68667 [details] [review] Bug 17989: Centralize bad template check The bad template check in get_template_and_user can be removed, since this check has been added in gettemplate on bug 18010. The check moves here to a new sub in C4::Templates. And will be extended in a follow-up. Removed unused variable $path in gettemplatefile. Test plan: [1] Run t/db_dependent/Auth.t (get_template_and_user bad calls). [2] Run t/db_dependent/Templates.t (bad call to gettemplate). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 68668 [details] [review] Bug 17989: Extend bad template check The check is now extended by only allowing templates from the regular Koha template directories and additional plugin directories as defined in koha-conf.xml. Note: In order to prevent an uninitialized warning on $theme from sub themelanguage for a not-existing file, I added a trivial assignment. Will get further attention in a follow-up. Test plan: [1] Run t/db_dependent/Auth.t [2] Run t/db_dependent/Templates.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 68669 [details] [review] Bug 17989: Include full path logic in _get_template_file Similar to the full path test in sub themelanguage, this patch makes a change in _get_template_file. This allows you to pass a template outside the modules directory to get_template_and_user. (Note: the sub badtemplatecheck already blocks bad paths.) Especially, this would be helpful for plugins using templates. As can be seen in Templates.pm, a change was made earlier to overwrite the filename for a plugin in sub gettemplate. This exception can now be removed. Also note the small change in Koha/Plugin/Base.pm; mbf_path is already absolute and if we pass a full path, we do not need it. This allows use of a regular Koha template or a shared template between plugins (as long as badtemplatecheck allows the path). What are the side-effects of this change? [1] We should not pass absolute paths if we mean relative ones. A follow-up patch deals with one occurrence in the codebase. No regressions for regular use. [2] Plugins can call get_template_and_user directly or go via get_template in Koha/Plugin/Base (absolute paths don't go via mbf_path). Note: replaced two single quotes in Auth.pm to show template name in test description. Test plan: [1] Open some page on OPAC or staff client to trigger a template. [2] Run t/db_dependent/Auth.t to verify not allowing some bad templates. [3] Run t/db_dependent/Templates.t to verify an absolute path. [4] Run t/db_dependent/Plugins.t to verify using templates in a plugin. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 68670 [details] [review] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates *** WARNING: Verbatim paragraph in NAME section at line 28 in file C4/Templates.pm Test plan: Run podchecker on C4::Templates and verify that the warning is resolved. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 68671 [details] [review] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. [2] Script svc/members/search gets its template path from a URL parameter. When you git grep json_template, you will have an idea what values it should receive. Test plan: [1] Go intranet home page. Search for patrons in the combo box. Results? [2] Try the URL /cgi-bin/koha/svc/members/search. Should trigger a 500 error with Bad template path in the logs. [3] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 68672 [details] [review] Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t SuggestionEngine_ExplodedTerms.t uses paths like /../.. or /.. in order to find a template in opac-tmpl. Sub badtemplatecheck does not like these paths. Using Cwd::abs_path to resolve the situation (rel2abs does not). Test plan: Run again t/db_dependent/SuggestionEngine_ExplodedTerms.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment on attachment 68671 [details] [review] Bug 17989: Final changes Review of attachment 68671 [details] [review]: ----------------------------------------------------------------- ::: svc/members/search @@ +28,5 @@ > use Koha::Patrons; > > my $input = new CGI; > +my $template_path = $input->param('template_path'); > +if( !$template_path || $template_path =~ /^\/|\.\./ ) { Why do we need to check that here? It is already checked in badtemplatecheck.
Created attachment 68842 [details] [review] Bug 17989: Final changes [1] The template sco/printslip.tt is in the regular modules directory. Should not be prefixed with a slash. Test plan: [1] Try to find another occurrence just like the one corrected in sco/printslip.pl where an absolute path is passed to gettemplate or get_template_and_user. I already tried several regex variations while git grepping template_name, but you may still find one.. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: Removed the changes for svc/members/search. Not needed. Commit message adjusted accordingly.
Created attachment 68843 [details] [review] Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t SuggestionEngine_ExplodedTerms.t uses paths like /../.. or /.. in order to find a template in opac-tmpl. Sub badtemplatecheck does not like these paths. Using Cwd::abs_path to resolve the situation (rel2abs does not). Test plan: Run again t/db_dependent/SuggestionEngine_ExplodedTerms.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #32) > Comment on attachment 68671 [details] [review] [review] > Bug 17989: Final changes > > Review of attachment 68671 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: svc/members/search > @@ +28,5 @@ > > use Koha::Patrons; > > > > my $input = new CGI; > > +my $template_path = $input->param('template_path'); > > +if( !$template_path || $template_path =~ /^\/|\.\./ ) { > > Why do we need to check that here? It is already checked in badtemplatecheck. No, it is not needed. Removed it.
Pushed to master for 17.11, thanks to everybody involved!