Bug 7013 - required format is not enforced for authorized values
Summary: required format is not enforced for authorized values
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Target Milestone: ---
Assignee: Christophe Croullebois
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
: 10237 (view as bug list)
Depends on: 7450
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-12 14:46 UTC by Gaetan Boisson
Modified: 2018-06-06 16:03 UTC (History)
8 users (show)

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


Attachments
proposed patch (9.22 KB, patch)
2011-12-05 09:41 UTC, Adrien SAURAT
Details | Diff | Splinter Review
patch (9.08 KB, patch)
2011-12-05 13:17 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 7013 : Fixing sql injection problems (2.15 KB, patch)
2011-12-05 15:49 UTC, Chris Cormack
Details | Diff | Splinter Review
proposed patch with client-side test (11.33 KB, patch)
2011-12-22 09:38 UTC, Adrien SAURAT
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gaetan Boisson 2011-10-12 14:46:24 UTC
When creating a new authorized value in:
Home › Administration › Authorized Values › New authorized value
There is no control on what the user enters as the authorized value.
However, entering a value which includes spaces or any special character(quotes, accents, etc.) will create problems at other places in the admin interface.
Proper instructions are also required on the page (with the list of allowed characters) as well as in the manual.

Here is an example of issue this can generate:

use CCODE as the type used to limit branch transfer
create CCODE values like "compact disc", "children's music"
go to Home › Administration › Set Library Checkin and Transfer Policy
tabs are not working because of the malformed CCODE value.
Comment 1 Adrien SAURAT 2011-12-05 09:41:17 UTC Comment hidden (obsolete)
Comment 2 Adrien SAURAT 2011-12-05 09:42:18 UTC
Two sentences created (a hint and an error message) in the proposed patch, to be translated.
Comment 3 Sébastien Marie 2011-12-05 10:17:25 UTC
Comment on attachment 6577 [details] [review]
proposed patch

This patch is vulnerable to SQL injection.

Please use placeholders in SQL query (using prepare/execute), instead of $variable.
Comment 4 Adrien SAURAT 2011-12-05 11:07:29 UTC
I didn't change the SQL parts, they could be indeed enhanced but it would be better to make a specific bug about this, to make things clear.

But thanks to your remark, I noticed a version problem :/ the script has probably been modified as I was working on this. I'll handle the conflicts and upload a new patch.
Comment 5 Adrien SAURAT 2011-12-05 13:17:32 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-12-05 15:47:27 UTC
I agree with comment 3, this code is still vulnerable to sql injection. 

I will add a follow up, but I would encourage all developers to fix any bad coding practices like this that they say, as they change code. It is the perfect opportunity and really, there is no good reason to ever not use sql placeholders.
Comment 7 Chris Cormack 2011-12-05 15:49:00 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2011-12-05 20:14:09 UTC
I tried submitting an invalid value for a CCODE and did get an error message, but the error messages was on the Authorised Values home page (/cgi-bin/koha/admin/authorised_values.pl) rather than on the submission form for CCODE (/cgi-bin/koha/admin/authorised_values.pl?op=add_form&category=CCODE).

The error messages should show on the same page from which the user submitted the invalid value, ideally with all the other values retained in the form so that they don't have to be re-typed.
Comment 9 Adrien SAURAT 2011-12-06 08:16:01 UTC
@Chris: Ok!

@Owen: Yes, to be consistent I used the same behaviour as another error message already implemented (when you try to use a value already in use). It would be better to do everything on the same page, I agree.
I don't have time for this right now but I'll take a look asap.
Comment 10 Adrien SAURAT 2011-12-22 09:38:11 UTC
Created attachment 6921 [details] [review]
proposed patch with client-side test

I left the server-side controls (check if duplicate entry), but added a client-side control with javascript alert window, as seen in Patron categories GUI.
Comment 11 Adrien SAURAT 2011-12-22 10:41:54 UTC
Well, in fact it seems that preventing spaces in every authorized value could cause more harm than good. Only in "ccode" should this format be really enforced.
Comment 12 Gaetan Boisson 2011-12-22 10:47:46 UTC
What i understood so far is that constraining the authorised value to a formated code can generate problems elsewhere, particularly if we want the value to be indexed and searchable.

The problem could be shifted from the enforcement of a required format to the fix of the places where the interface is broken when an autorised value contains spaces or other problematic characters.

Or as Adrien suggests, just enforce a format for CCODE. (But then what if other values break the interface? do we have other places where authorized values appear in tabs?)
Comment 13 Katrin Fischer 2011-12-22 10:50:23 UTC
I am not sure I agree here. Authorized value codes are codes for internal use. To me it makes a lot of sense to restrict them to alphanumeric values and perhaps underscored. Display and search should use the description?
Where do we need spaces?
Comment 14 Laurence Rault 2011-12-22 11:39:53 UTC
Unless I mistake, Zebra search currently does not use description but AV code. 
We set AV=description for libraries that want to search on indexes like location or specific indexes in full words, as patrons never know the codes.

Enforcing a code for these values will imply a loss of search functionalities, imho.

Maybe CCODE should be a specific AV, as it is involved in specific rules or parameters, but I'm not sure I agree to enforce a code for all AV.

Unless zebra is able to search in description in a relatively short delay ? Solr already do that.
Comment 15 Katrin Fischer 2011-12-22 11:44:26 UTC
I can see why it would make sense to do that, but I don't think we should use internal codes like this. The search should be fixed in this case.
We have search options in advanced search to translate codes into descriptions. And we do the same when searching for branches, in advanced search and in facets. Display description, search for code.
Comment 16 Katrin Fischer 2012-01-17 09:05:00 UTC
Now as this has moved into 'In Discussion' I think it will take a while to be resolved. I think we should fix the security problem found asap.
Comment 17 Adrien SAURAT 2012-01-17 11:07:57 UTC
A patch fixing the security issue (missing placeholders) has been proposed in bug 7450.
Comment 18 Paul Poulain 2012-02-10 14:00:40 UTC
In discussion = it's quite unclear to me how to start the discussion. Could one of you start the discussion wiki page & the thread on koha-devel & koha mailing list ?
Comment 19 Paul Poulain 2012-02-10 14:01:21 UTC
the wiki link to start the discussion = http://wiki.koha-community.org/wiki/Bug_and_Enhancement_Discussion
Comment 20 Katrin Fischer 2013-08-05 09:06:46 UTC
I think in order to fix this properly we need to fix how MANUAL_INV works. It uses the code as description and the description for the amount. We could use the code as a real code and then the description as description... and the opac description for the amount. But it would still be bending something to do what it was not supposed to do in the first place.

And do we still need to fix the security issue??
Comment 21 Katrin Fischer 2013-08-05 09:09:48 UTC
*** Bug 10237 has been marked as a duplicate of this bug. ***