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.
Created attachment 6577 [details] [review]
Two sentences created (a hint and an error message) in the proposed patch, to be translated.
Comment on attachment 6577 [details] [review]
This patch is vulnerable to SQL injection.
Please use placeholders in SQL query (using prepare/execute), instead of $variable.
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.
Created attachment 6579 [details] [review]
Diff show a lot of modified lines because the indentation of a few blocks changed, but I actually added a just few lines.
What I did :
- added the regex "$new_authorised_value =~ /^[a-zA-Z0-9\-_]+$/" to allow only letters, numbers, dash and underscore for a new value;
- added a new hint next to the input box;
- added a new error message when the user types an incorrect value.
Also removed a few trailing spaces (they caused warnings when applying the patch).
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.
Created attachment 6582 [details] [review]
Bug 7013 : Fixing sql injection problems
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.
@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.
Created attachment 6921 [details] [review]
proposed patch with client-side test
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.
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?)
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?
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.
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.
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.
A patch fixing the security issue (missing placeholders) has been proposed in bug 7450.
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 ?
the wiki link to start the discussion = http://wiki.koha-community.org/wiki/Bug_and_Enhancement_Discussion
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??
*** Bug 10237 has been marked as a duplicate of this bug. ***
On bug 17216 it was created the authorised_value_categories table. Probably should be added a new column there for the description of each category of AVs, and therefore add to the UI a Modify button to change it.
Sorry, it was for bug 23861 :(