Bug 2847 - Use HTML escape in templates where appropriate
Summary: Use HTML escape in templates where appropriate
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: 3.6
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Owen Leonard
QA Contact:
URL:
Keywords:
Depends on: 3652
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-05 02:34 UTC by Rick Welykochy
Modified: 2013-12-05 19:59 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch for categorie.tmpl (9.55 KB, patch)
2009-04-19 22:48 UTC, Chris Cormack
Details | Diff | Splinter Review
Corrected patch for categorie.pl (10.48 KB, application/octet-stream)
2009-04-20 16:16 UTC, Chris Cormack
Details
Proposed fix (6.31 KB, patch)
2011-11-11 17:40 UTC, Owen Leonard
Details | Diff | Splinter Review
Incremental fix for Bug 2847, Use HTML escape in templates where appropriate (6.33 KB, patch)
2011-11-12 06:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Proposed fix for 3.4.x (5.95 KB, patch)
2011-11-23 14:47 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:57:08 UTC


---- Reported by rick@praxis.com.au 2008-12-05 02:34:55 ----

The values of input fields, the names and values in tags in general, and the passing
of data via HTML all require HTML escaping, i.e. translating < to &lt; and " to &quot.


Here is a concrete example of the HTML escape problem.

 1. In the Modify Patron script members/memberentry.pl, the borrower attributes can
    be modified. And in the Admin area the attribute types (or codes) can be defined.

 2. Create an attribute whose code is this: REF "B"

 3. The attribute can be created but it cannot be modified, because the HTML for
    the modification script says this: <input ... value="REF "B"">

    The browser interprets the value as "REF ", not 'REF "B"'

 4. Once that patron attribute code is in the system, all hell can break lose. The
    attributes for that patron can no longer be edited. The error message is something
    like the following in Koha/3.0:

        The following fields are wrong. Please fix them.
        * The attribute value REF /slarty is already is use by another patron record.

    The script should have been given REF "8" as the field name, but got "REF" instead.

 5. The fix is to add the escape parameter to the TMPL_VAR tag:

    <input ... value="<!--TMPL_VAR Name="something" escape="html" -->" />

 6. Other tags present the same problem:

    <option> esp. the value= attribute of the tag

    In general, look for the following construct and add the escape="html" attribute to
    the TMPL_VAR tag:

        <whatever ... value="<!--TMPL_VAR Name="something"--> />



---- Additional Comments From cjaeger@dce.k12.wi.us 2009-04-19 22:48:24 ----

Created an attachment
Patch for categorie.tmpl

Patch for categorie.tmpl 
Many templates still need to be updated for this bug.



---- Additional Comments From cjaeger@dce.k12.wi.us 2009-04-20 16:16:47 ----

Created an attachment
Corrected patch for categorie.pl

Correct html and url escaping in categorie.pl template. Use this patch and not the previous.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:57 UTC  ---

This bug was previously known as _bug_ 2847 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=2847
Imported an attachment (id=887)
Imported an attachment (id=888)

Actual time not defined. Setting to 0.0
The original submitter of attachment 887 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 888 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Owen Leonard 2011-11-11 17:40:57 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-11-12 06:25:53 UTC
Created attachment 6299 [details] [review]
Incremental fix for Bug 2847, Use HTML escape in templates where appropriate

Fixes for output in a couple of acquisitions templates where
user-generated data should be escaped. This instances were found
by creating a vendor name like "Baker & Taylor" and finding
that the ampersand was not escaped, causing validation errors.

This patch also consolidates multiple <script> blocks which
do not need to be separate and corrects a couple of unclosed
<input> tags.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 3 Paul Poulain 2011-11-18 22:15:52 UTC
QA comment: I'm not fan of mixing things in patches (html/js), but it's small & announced, so passed QA

Patch pushed, please test
Comment 4 Chris Nighswonger 2011-11-20 02:24:54 UTC
This patch does not apply cleanly to the 3.4.x branch. If it should, please rebase over that branch and attach the corrected path to this bug report.

cnighs@erasmus:~/koha.3.2.test (3.4.x)$ ./get_commits.pl -b 3.4.x -l 35234b478a
Shall I apply the bugfix(s) for 2847? (Y/n)y
Automatic cherry-pick failed.  After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>'
and commit the result with:

        git commit -c bfe06ef399da831e5602784fbe54cea6dfc1ab65
Comment 5 Paul Poulain 2011-11-23 11:26:44 UTC
is it just a problem on 3.4 ? it's OK for 3.6 ?
In this case, I think we can stay without this fix in 3.4 : it's a security issue, I agree, but:
* it's staff related, so, to exploit such a bug, one would first need to have a valid login, so, the risk is low according to me.
* 3.4 EOL is probably close
Comment 6 Chris Nighswonger 2011-11-23 13:07:05 UTC
(In reply to comment #5)
> is it just a problem on 3.4 ? it's OK for 3.6 ?
> In this case, I think we can stay without this fix in 3.4 : it's a security
> issue, I agree, but:
> * it's staff related, so, to exploit such a bug, one would first need to have a
> valid login, so, the risk is low according to me.

IMHO any security issue should be backported into any currently maintained branch.

> * 3.4 EOL is probably close

Not as close as we might imagine. As long as commits will apply, I plan on keeping 3.4.x alive until we reach a year from 3.6.x release.
Comment 7 Owen Leonard 2011-11-23 14:47:45 UTC
Created attachment 6380 [details] [review]
Proposed fix for 3.4.x
Comment 8 Jared Camins-Esakov 2012-05-23 12:32:22 UTC
Included in the 3.6 branch prior to 3.6.4.