GNOME Bugzilla – Bug 677871
EBookBackendSqliteDB - Escape SQL strings
Last modified: 2013-09-14 16:55:47 UTC
Created attachment 216128 [details] [review] EBookBackendSqliteDB - Fix the 'is' operator e_book_client_get_contacts_uids() doesn't find contacts if the query contains single quotes, as for instance in this query: (is "given_name" "Zahra'a"). Actually it entirely fails to escape single quotes, opening the door for SQL injection attacks to the summary database. The attached patch series shall fix the issue.
Created attachment 216129 [details] [review] EBookBackendSqliteDB - Extract convert_string_value()
Created attachment 216130 [details] [review] EBookBackendSqliteDB - Use switch instead of nested ifs
Created attachment 216131 [details] [review] EBookBackendSqliteDB - Use GString instead of g_strdup_printf()
Created attachment 216132 [details] [review] EBookBackendSqliteDB - Escape SQL strings \O/ finally, that's the actual fix
Created attachment 216162 [details] [review] EBookBackendSqliteDB - Use GString instead of g_strdup_printf() correctly place percent signs for beginswith and endswith
Personally, I prefer one patch instead of five. Check your bugzilla mail for why, and that I usually do not care how you got to the result, because only the result matter. Patch1: looks ok. Patch2: please use gchar, not char; otherwise looks good Patch3: I do not see much difference, but no problem with it (compiler can take care of checking for all enum values, maybe that's why) Patch4: char versus gchar again; escape_modifier[] no need for static, and make it *escape_modifier sizeof should be strlen escape_modified_needed might be escape_modifier_needed ('d' to 'r') what about using other than '\\'? It can have its own meaning for the DB engine, thus I suggest '^'. Itll be also easier to read in debugger Patch5: it makes code harder to read, I would do that.
well, attachment 216162 [details] [review] (aka. patch 5 - seems the review plugin doesn't deal with attachment deprecation?) prepares the fix found in patch 4. I'd expect more inefficient and confusing code by not using GString. Well, but I can try.
Aha, I didn't realize the patch 4 and patch 5 are in an opposite order. I applied all those patches and committed them in once: Created commit 5cff7e6 in eds master (3.5.92+)
Actually, some patches are not committed in the same version as they are attached here, I changed things according to my comments in comment #6, except of the one for patch 5, which should be "would _not_ do it", but it was out of question afterwards anyway.