Bug 17989 - Stricter control on source directory for html templates
Summary: Stricter control on source directory for html templates
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18010
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-25 09:56 UTC by Marcel de Rooy
Modified: 2017-11-01 16:15 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17989: Centralize bad template check (3.62 KB, patch)
2017-07-26 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Extend bad template check (4.11 KB, patch)
2017-07-26 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Include full path logic in _get_template_file (10.69 KB, patch)
2017-07-26 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates (955 bytes, patch)
2017-07-26 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Final changes (2.37 KB, patch)
2017-07-26 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Centralize bad template check (3.62 KB, patch)
2017-08-21 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Extend bad template check (4.10 KB, patch)
2017-08-21 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Include full path logic in _get_template_file (10.69 KB, patch)
2017-08-21 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates (955 bytes, patch)
2017-08-21 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Final changes (2.37 KB, patch)
2017-08-21 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17989: Centralize bad template check (3.65 KB, patch)
2017-08-22 10:58 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17989: Extend bad template check (4.13 KB, patch)
2017-08-22 10:58 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17989: Include full path logic in _get_template_file (10.72 KB, patch)
2017-08-22 10:58 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates (984 bytes, patch)
2017-08-22 10:58 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17989: Final changes (2.40 KB, patch)
2017-08-22 10:58 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17989: Centralize bad template check (3.62 KB, patch)
2017-10-02 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Extend bad template check (4.10 KB, patch)
2017-10-02 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Include full path logic in _get_template_file (10.69 KB, patch)
2017-10-02 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates (1011 bytes, patch)
2017-10-02 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Final changes (2.42 KB, patch)
2017-10-02 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t (1.26 KB, patch)
2017-10-27 05:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: Centralize bad template check (3.64 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: Extend bad template check (4.12 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: Include full path logic in _get_template_file (10.71 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: Resolve "verbatim paragraph in NAME section" warning in C4::Templates (1.00 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: Final changes (2.44 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t (1.28 KB, patch)
2017-10-27 09:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17989: Final changes (1.52 KB, patch)
2017-10-30 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17989: (QA follow-up) Replace bad dots in SuggestionEngine_ExplodedTerms.t (1.32 KB, patch)
2017-10-30 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-01-25 09:56:39 UTC

    
Comment 1 Marcel de Rooy 2017-07-26 13:08:09 UTC
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>
Comment 2 Marcel de Rooy 2017-07-26 13:08:13 UTC
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>
Comment 3 Marcel de Rooy 2017-07-26 13:08:17 UTC
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>
Comment 4 Marcel de Rooy 2017-07-26 13:08:20 UTC
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>
Comment 5 Marcel de Rooy 2017-07-26 13:08:24 UTC
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>
Comment 6 Marcel de Rooy 2017-08-21 14:20:36 UTC
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>
Comment 7 Marcel de Rooy 2017-08-21 14:20:40 UTC
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>
Comment 8 Marcel de Rooy 2017-08-21 14:20:43 UTC
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>
Comment 9 Marcel de Rooy 2017-08-21 14:20:47 UTC
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>
Comment 10 Marcel de Rooy 2017-08-21 14:20:51 UTC
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>
Comment 11 Marcel de Rooy 2017-08-21 14:21:07 UTC
Trivial rebase
Comment 12 Josef Moravec 2017-08-22 10:58:27 UTC
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>
Comment 13 Josef Moravec 2017-08-22 10:58:30 UTC
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>
Comment 14 Josef Moravec 2017-08-22 10:58:33 UTC
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>
Comment 15 Josef Moravec 2017-08-22 10:58:36 UTC
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>
Comment 16 Josef Moravec 2017-08-22 10:58:39 UTC
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>
Comment 17 Marcel de Rooy 2017-08-22 11:48:31 UTC
(In reply to Josef Moravec from comment #16)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Great !
Comment 18 Marcel de Rooy 2017-10-02 10:02:35 UTC
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>
Comment 19 Marcel de Rooy 2017-10-02 10:02:39 UTC
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>
Comment 20 Marcel de Rooy 2017-10-02 10:02:43 UTC
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>
Comment 21 Marcel de Rooy 2017-10-02 10:02:46 UTC
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>
Comment 22 Marcel de Rooy 2017-10-02 10:02:50 UTC
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>
Comment 23 Marcel de Rooy 2017-10-02 10:03:09 UTC
Simple rebase
Comment 24 Nick Clemens 2017-10-26 14:34:15 UTC
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
Comment 25 Marcel de Rooy 2017-10-27 05:56:52 UTC
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>
Comment 26 Nick Clemens 2017-10-27 09:41:29 UTC
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>
Comment 27 Nick Clemens 2017-10-27 09:41:32 UTC
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>
Comment 28 Nick Clemens 2017-10-27 09:41:35 UTC
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>
Comment 29 Nick Clemens 2017-10-27 09:41:38 UTC
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>
Comment 30 Nick Clemens 2017-10-27 09:41:40 UTC
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>
Comment 31 Nick Clemens 2017-10-27 09:41:43 UTC
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 32 Jonathan Druart 2017-10-27 16:40:16 UTC
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.
Comment 33 Marcel de Rooy 2017-10-30 10:44:27 UTC
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.
Comment 34 Marcel de Rooy 2017-10-30 10:44:31 UTC
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>
Comment 35 Marcel de Rooy 2017-10-30 10:45:13 UTC
(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.
Comment 36 Jonathan Druart 2017-11-01 16:15:00 UTC
Pushed to master for 17.11, thanks to everybody involved!