(In reply to Florian Weimer from comment #0)
> Currently, gom does not protect its SQL generation logic against special
> characters in identifiers. 

What identifiers, specifically?

> Characters such as '![] should be rejected to
> prevent any risk of SQL injection.  There are some places which do not seem
> to be able to cope with spaces in identifiers because the identifiers are
> not quoted at all.

Identifiers for tables are set by the object classes, so they could potentially be problematic. But those would be set in code, not coming from user input.

Similarly, all the object properties names that will correspond to column names in SQL, must follow a separate scheme (from g_param_spec_internal()'s docs):
 * A property name consists of segments consisting of ASCII letters and
 * digits, separated by either the '-' or '_' character. The first
 * character of a property name must be a letter.

So all valid property names will be valid SQL column names, and you won't be able to have invalid column names running in code.

So, in short, which identifiers exactly are the problem? And what entry points did you find for those identifiers that could be passed from users but invalid?

(In reply to Bastien Nocera from comment #1)
> (In reply to Florian Weimer from comment #0)
> > Characters such as '![] should be rejected to
> > prevent any risk of SQL injection.  There are some places which do not seem
> > to be able to cope with spaces in identifiers because the identifiers are
> > not quoted at all.
> 
> Identifiers for tables are set by the object classes, so they could
> potentially be problematic. But those would be set in code, not coming from
> user input.

Indeed, I tried setting the table name to "episodes;" and I got:

assertion failed (error == NULL): sqlite3_prepare_v2 failed: near ";": syntax error: INSERT INTO episodes; ('series-id', 'imdb-id', 'season-number', 'episode-number', 'episode-name') VALUES (?, ?, ?, ?, ?); ("gom-error-quark", 2)

> Similarly, all the object properties names that will correspond to column
> names in SQL, must follow a separate scheme (from g_param_spec_internal()'s
> docs):
>  * A property name consists of segments consisting of ASCII letters and
>  * digits, separated by either the '-' or '_' character. The first
>  * character of a property name must be a letter.
> 
> So all valid property names will be valid SQL column names, and you won't be
> able to have invalid column names running in code.

This is not really how it works.

I tried it, and it works just fine to use property names with ";", "?", spaces, etc...

What happens though is that when passing them through GParamSpec, they names get mangled and all those special characters get replaced by "-" characters.

As a result, if you have a resource object with a "season-number;" property, it will point to a "season-number-" column, and everything works just fine.

Except for GomSorting object, these don't work at all, because when I created it, I didn't use GParamSpec at all for it. I have a patch ready to fix that. :)

Created attachment 308837 [details] [review]
sorting: Use a GParamSpec internally

Other Gom objects already do this, for example GomFilter.

One of the good things with this is that when getting a GParamSpec, the
property id is sanitized, so that it won't contain any special
characters like ";", "?", or other things which could cause SQL
injections.

The last thing I can see which could be problematic is the values passed to SQL statements.

But AFAICS, we always use the sqlite3_bind_* functions to pass values to SQL statements, so we should be ok there.

Comment on attachment 308837 [details] [review]
sorting: Use a GParamSpec internally

Bastien is sitting next to me, reviewed this patch and told me to just push it.